diff --git a/changelog.d/tsk-2mgpp7-fix-coderabbit-auto-reply-bypass.md b/changelog.d/tsk-2mgpp7-fix-coderabbit-auto-reply-bypass.md new file mode 100644 index 000000000..bdbf7e149 --- /dev/null +++ b/changelog.d/tsk-2mgpp7-fix-coderabbit-auto-reply-bypass.md @@ -0,0 +1,5 @@ +### Fixed +- Bot-review gate no longer treats CodeRabbit's auto-reply notice (``) as a real review. The notice is posted when a `@coderabbitai review` trigger is acknowledged but the review has not yet run (rate-limit recovery). Previously this auto-reply flipped the gate green with zero review content. +- `is_real_item()` now requires positive evidence of review content (APPROVED/CHANGES_REQUESTED state, walkthrough with Run ID + signals, inline findings, or structured review body with code/finding markers). The fail-open fallback that counted any unrecognised non-empty CodeRabbit comment as real has been removed. +- Added `is_coderabbit_auto_reply()` detector and folded it into `is_coderabbit_scaffolding()` as a third per-fragment detector, maintaining detector isolation so one regressing cannot mask another. +- The `bot-review-allow` waiver label continues to work for deliberate lead overrides of stub-only verdicts. \ No newline at end of file diff --git a/scripts/check_bot_review.py b/scripts/check_bot_review.py index f70ba24ff..68fa5524d 100755 --- a/scripts/check_bot_review.py +++ b/scripts/check_bot_review.py @@ -72,8 +72,10 @@ # HTML comment markers and phrases that identify CodeRabbit auto-generated # scaffolding that must never read as a real review: the acknowledgement -# reply posted when a @coderabbitai full review trigger is accepted but -# produces no review, and the failure notice posted when a review run fails. +# reply posted when a @coderabbitai review trigger is accepted but +# produces no review, the failure notice posted when a review run fails, +# and the auto-reply notice posted when a @coderabbitai review trigger is +# acknowledged but the review has not yet run (rate-limit recovery notice). # The auto-summary marker is intentionally NOT listed here: in this repo it # is the walkthrough issue comment, the only review artifact on a clean PR, # and is handled by the walkthrough detector below. @@ -88,12 +90,16 @@ r"failure by coderabbit\.ai|Review failed", re.IGNORECASE, ) +CODERABBIT_AUTO_REPLY_RE = re.compile( + r"", + re.IGNORECASE, +) CODERABBIT_AUTO_SUMMARY_RE = re.compile( r"", re.IGNORECASE, ) CODERABBIT_SCAFFOLDING_RE = re.compile( - rf"{CODERABBIT_ACKNOWLEDGEMENT_RE.pattern}|{CODERABBIT_FAILURE_RE.pattern}", + rf"{CODERABBIT_ACKNOWLEDGEMENT_RE.pattern}|{CODERABBIT_FAILURE_RE.pattern}|{CODERABBIT_AUTO_REPLY_RE.pattern}", re.IGNORECASE, ) @@ -248,7 +254,11 @@ def is_coderabbit_scaffolding(body: str | None) -> bool: single stub check see the same coverage; internal callers should prefer the per-fragment detectors so a regression in one cannot be hidden by the other. """ - return is_coderabbit_acknowledgement(body) or is_coderabbit_failure_notice(body) + return ( + is_coderabbit_acknowledgement(body) + or is_coderabbit_failure_notice(body) + or is_coderabbit_auto_reply(body) + ) def is_coderabbit_acknowledgement(body: str | None) -> bool: @@ -281,6 +291,16 @@ def is_coderabbit_failure_notice(body: str | None) -> bool: return bool(CODERABBIT_FAILURE_RE.search(body)) +def is_coderabbit_auto_reply(body: str | None) -> bool: + """Return True if a body is CodeRabbit's auto-reply notice -- posted when + a @coderabbitai review trigger is acknowledged but the review has not yet + run (rate-limit recovery notice). This announces that a review has NOT + happened yet and must not read as a real review.""" + if not body: + return False + return bool(CODERABBIT_AUTO_REPLY_RE.search(body)) + + def is_coderabbit_walkthrough(body: str | None) -> bool: """Return True if a body is a CodeRabbit walkthrough issue comment that represents a real review. @@ -371,11 +391,13 @@ def is_real_item(item: CRItem) -> bool: A rate-limit stub is never real. Review objects with state APPROVED or CHANGES_REQUESTED are real regardless of body content (the review state itself is the substantive signal). CodeRabbit scaffolding (acknowledgement - reply / failure notice) is never real. For issue comments carrying the - auto-summary marker, the walkthrough detector applies: a Run ID plus at - least one signal (quota-decrement line, no-actionable phrase, or - Files-processed list) means a real review ran. Other comments are real - when they carry non-empty, non-stub body text. + reply / failure notice / auto-reply) is never real. For issue comments + carrying the auto-summary marker, the walkthrough detector applies: a Run + ID plus at least one signal (quota-decrement line, no-actionable phrase, + or Files-processed list) means a real review ran. Review comments + (line-level discussion threads) with non-empty body are inline findings + and count as real. Other comments require positive evidence of review + content -- substantive body text that is not marker-only scaffolding. """ if is_rate_limit_stub(item.body): return False @@ -383,11 +405,35 @@ def is_real_item(item: CRItem) -> bool: state = (item.review_state or "").upper() if state in ("APPROVED", "CHANGES_REQUESTED"): return True + # COMMENTED reviews fall through to body evidence checks below if is_coderabbit_scaffolding(item.body): return False if not item.is_review and is_coderabbit_auto_summary(item.body): return is_coderabbit_walkthrough(item.body) - return bool(item.body and item.body.strip()) + # Review comments (line-level) with non-empty body are inline findings. + # Other comments need positive evidence: substantive body that is not + # just a marker or short automated notice. + if item.body and item.body.strip(): + body = item.body.strip() + # Skip if body is only an HTML comment marker (scaffolding that + # wasn't caught by the specific detectors above). + if body.startswith("") and "\n" not in body: + return False + # Positive evidence: body contains code references, line numbers, + # file paths, finding keywords, or structured review sections. + # Scaffolding (status notices, action confirmations) lacks these. + if re.search( + r"(line\s+\d+|`[^`]+`|\[.*\]\(|#\d+|\.py|\.js|\.ts|\.json|" + r"suggest|fix|issue|bug|error|warn|TODO|FIXME|" + r"##\s*(Review|Findings|Summary|Changes|Walkthrough)|" + r"###\s*(Line|File|Change|Issue|Finding))", + body, + re.IGNORECASE, + ): + return True + # No positive evidence found -- likely scaffolding or status notice. + return False + return False def _is_coderabbit(user: dict | None) -> bool: diff --git a/tests/scripts/test_check_bot_review.py b/tests/scripts/test_check_bot_review.py index 12f660685..b40a6e6a4 100644 --- a/tests/scripts/test_check_bot_review.py +++ b/tests/scripts/test_check_bot_review.py @@ -1070,10 +1070,39 @@ class TestDetectorIsolation: Each body below is matched by EXACTLY one detector. Neutering that detector loses protection for its body but leaves the other detectors' bodies caught, - so no single neuter can stay green by leaning on a different detector.""" + so no single neuter can stay green by leaning on a different detector. + + With the fail-closed default (positive evidence required), pure stub bodies + that lack review content (no findings, no Run ID + signals, no code refs) + are still rejected by the default even when their specific detector is + neutered. The isolation property is verified by ensuring other detectors + still catch their respective stubs, and by using bodies with positive + evidence in the neutering tests where the detector is the only thing + keeping them red. + """ RL_BODY = "Review rate limited. Please try again later." FAILURE_BODY = "Review failed by coderabbit.ai" + # Bodies with positive evidence that would be real EXCEPT for the specific + # stub detector. Used to verify isolation: when the detector is neutered, + # these should pass (protection lost), while other detectors' stubs still fail. + RL_BODY_WITH_FINDINGS = ( + "## Review\n\n### Findings\n\nLine 42: potential memory leak here.\n\n" + "Note: review rate limited for next run." + ) + ACK_BODY_WITH_FINDINGS = ( + "\n" + "## Review\n\n### Findings\n\nLine 42: potential memory leak here." + ) + FAILURE_BODY_WITH_FINDINGS = ( + "Review failed by coderabbit.ai\n\n" + "## Review\n\n### Findings\n\nLine 42: potential memory leak here." + ) + AUTO_REPLY_BODY_WITH_FINDINGS = ( + "\n" + "Your plan includes PR reviews subject to rate limits. Reviews are available now.\n\n" + "## Review\n\n### Findings\n\nLine 42: potential memory leak here." + ) def test_rate_limit_body_rejected(self, check_mod) -> None: item = check_mod.CRItem(id=1, body=self.RL_BODY, is_review=True, review_state="COMMENTED") @@ -1081,10 +1110,21 @@ def test_rate_limit_body_rejected(self, check_mod) -> None: def test_neutering_rate_limit_loses_only_its_protection(self, check_mod) -> None: with patch.object(check_mod, "is_rate_limit_stub", return_value=False): - rl = check_mod.CRItem(id=1, body=self.RL_BODY, is_review=True, review_state="COMMENTED") + # Body with findings that only the rate-limit detector blocks + rl = check_mod.CRItem( + id=1, body=self.RL_BODY_WITH_FINDINGS, is_review=True, review_state="COMMENTED" + ) assert check_mod.is_real_item(rl) is True # protection lost + # Other detectors' pure stubs still caught (by them or fail-closed) ack = check_mod.CRItem(id=2, body=ACK_BODY, is_review=True, review_state="COMMENTED") assert not check_mod.is_real_item(ack) # acknowledgement still caught + failure = check_mod.CRItem(id=3, body=self.FAILURE_BODY, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(failure) # failure notice still caught + auto_reply = check_mod.CRItem( + id=4, body="\nReviews available.", + is_review=True, review_state="COMMENTED", + ) + assert not check_mod.is_real_item(auto_reply) # auto-reply still caught def test_acknowledgement_body_rejected(self, check_mod) -> None: item = check_mod.CRItem(id=1, body=ACK_BODY, is_review=True, review_state="COMMENTED") @@ -1092,10 +1132,19 @@ def test_acknowledgement_body_rejected(self, check_mod) -> None: def test_neutering_acknowledgement_loses_only_its_protection(self, check_mod) -> None: with patch.object(check_mod, "is_coderabbit_acknowledgement", return_value=False): - ack = check_mod.CRItem(id=1, body=ACK_BODY, is_review=True, review_state="COMMENTED") + ack = check_mod.CRItem( + id=1, body=self.ACK_BODY_WITH_FINDINGS, is_review=True, review_state="COMMENTED" + ) assert check_mod.is_real_item(ack) is True # protection lost failure = check_mod.CRItem(id=2, body=self.FAILURE_BODY, is_review=True, review_state="COMMENTED") assert not check_mod.is_real_item(failure) # failure notice still caught + auto_reply = check_mod.CRItem( + id=3, body="\nReviews available.", + is_review=True, review_state="COMMENTED", + ) + assert not check_mod.is_real_item(auto_reply) # auto-reply still caught + rl = check_mod.CRItem(id=4, body=self.RL_BODY, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(rl) # rate-limit still caught def test_failure_notice_body_rejected(self, check_mod) -> None: item = check_mod.CRItem(id=1, body=self.FAILURE_BODY, is_review=True, review_state="COMMENTED") @@ -1103,24 +1152,59 @@ def test_failure_notice_body_rejected(self, check_mod) -> None: def test_neutering_failure_notice_loses_only_its_protection(self, check_mod) -> None: with patch.object(check_mod, "is_coderabbit_failure_notice", return_value=False): - failure = check_mod.CRItem(id=2, body=self.FAILURE_BODY, is_review=True, review_state="COMMENTED") + failure = check_mod.CRItem( + id=2, body=self.FAILURE_BODY_WITH_FINDINGS, is_review=True, review_state="COMMENTED" + ) assert check_mod.is_real_item(failure) is True # protection lost ack = check_mod.CRItem(id=1, body=ACK_BODY, is_review=True, review_state="COMMENTED") assert not check_mod.is_real_item(ack) # acknowledgement still caught + auto_reply = check_mod.CRItem( + id=3, body="\nReviews available.", + is_review=True, review_state="COMMENTED", + ) + assert not check_mod.is_real_item(auto_reply) # auto-reply still caught + rl = check_mod.CRItem(id=4, body=self.RL_BODY, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(rl) # rate-limit still caught + + def test_auto_reply_body_rejected(self, check_mod) -> None: + body = "\nReviews are available now." + item = check_mod.CRItem(id=1, body=body, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(item) - def test_neutering_every_detector_loses_every_protection(self, check_mod) -> None: - """The trap the audit caught, reproduced: neutering ALL stub detectors - must let EVERY stub kind through (green), not stay red on one because an - untested detector was left on. Each stub must flip independently.""" + def test_neutering_auto_reply_loses_only_its_protection(self, check_mod) -> None: + with patch.object(check_mod, "is_coderabbit_auto_reply", return_value=False): + auto_reply = check_mod.CRItem( + id=1, body=self.AUTO_REPLY_BODY_WITH_FINDINGS, is_review=True, review_state="COMMENTED" + ) + assert check_mod.is_real_item(auto_reply) is True # protection lost + ack = check_mod.CRItem(id=2, body=ACK_BODY, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(ack) # acknowledgement still caught + failure = check_mod.CRItem(id=3, body=self.FAILURE_BODY, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(failure) # failure notice still caught + rl = check_mod.CRItem(id=4, body=self.RL_BODY, is_review=True, review_state="COMMENTED") + assert not check_mod.is_real_item(rl) # rate-limit still caught + + def test_fail_closed_default_catches_stubs_without_positive_evidence(self, check_mod) -> None: + """Even with ALL stub detectors neutered, pure stub bodies that lack + positive evidence of review content are still rejected by the fail-closed + default. This is the key behavioral change: the default is now fail-closed, + not fail-open.""" with patch.object(check_mod, "is_rate_limit_stub", return_value=False), \ patch.object(check_mod, "is_coderabbit_acknowledgement", return_value=False), \ - patch.object(check_mod, "is_coderabbit_failure_notice", return_value=False): + patch.object(check_mod, "is_coderabbit_failure_notice", return_value=False), \ + patch.object(check_mod, "is_coderabbit_auto_reply", return_value=False): rl = check_mod.CRItem(id=1, body=self.RL_BODY, is_review=True, review_state="COMMENTED") ack = check_mod.CRItem(id=2, body=ACK_BODY, is_review=True, review_state="COMMENTED") failure = check_mod.CRItem(id=3, body=self.FAILURE_BODY, is_review=True, review_state="COMMENTED") - assert check_mod.is_real_item(rl) is True - assert check_mod.is_real_item(ack) is True - assert check_mod.is_real_item(failure) is True + auto_reply = check_mod.CRItem( + id=4, body="\nReviews available.", + is_review=True, review_state="COMMENTED", + ) + # All pure stubs still fail because they lack positive evidence + assert not check_mod.is_real_item(rl) + assert not check_mod.is_real_item(ack) + assert not check_mod.is_real_item(failure) + assert not check_mod.is_real_item(auto_reply) class TestIsCoderabbitZeroFindingReview: