This patch fixes a bug where review-patch.py would detect and report an error or warning only based off of the occurrence of the headers of the error and warning sections. This led to consistent false positives as often AI reviewers will create the header but put "none" or similar filler text within the following body.
This patch updates the code in order to check if the AI review has a body with error or warnings to fix and not just filler text. This is done by keeping track of whether the for loop parser is within an error or warning section; analyzing the first non-whitespace line within the section. If the first non-whitespace line matches known filler then the section can be ignored. It has been observed that if the AI includes filler then there is no actual concern. These changes were tested against 10+ markdown AI review outputs with several variations in formatting and filler text. The changes caught error or warning sections with actual concerns and successfully ignored sections containing only filler. Signed-off-by: Matthew Gee <[email protected]> --- devtools/ai/review-patch.py | 48 +++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/devtools/ai/review-patch.py b/devtools/ai/review-patch.py index 52601ac156..53ae7e6a4f 100755 --- a/devtools/ai/review-patch.py +++ b/devtools/ai/review-patch.py @@ -19,6 +19,7 @@ from email.message import EmailMessage from pathlib import Path from typing import Any, Iterator +from enum import Flag, auto from _common import ( PROVIDERS, @@ -120,6 +121,12 @@ EXIT_ERRORS = 3 +class ReviewParseState(Flag): + NORMAL = auto() + IN_ERROR = auto() + IN_WARNING = auto() + + def classify_review(review_text: str, output_format: str) -> int: """Classify review result and return appropriate exit code. @@ -147,22 +154,43 @@ def classify_review(review_text: str, output_format: str) -> int: pass # Fall through to text scanning if not has_errors and not has_warnings: - # Scan review text for severity indicators. - # Match section headers and inline markers across text/markdown/html. - for line in review_text.splitlines(): - stripped = line.strip().lower() - # Skip quoted patch context lines - if stripped.startswith(">") or stripped.startswith("diff --git"): + # Matches against error or warning section headers + rgx_header_match: str = r"(#+\s)?(\*+)?(<h[1-3]>)?{err_or_warn}" + # Matches against observed filler text + rgx_filler_match: str = r"(none(.)?$|\(must fix\)$|$)" + + curr_line: str + for curr_line in review_text.splitlines(): + stripped: str = curr_line.strip().lower() + + if ( + stripped.startswith(">") + or stripped.startswith("diff --git") + or stripped == "" + ): continue - if re.match(r"^(#{1,3}\s+)?(\*{0,2})error", stripped) or re.match( - r"^<h[1-3]>\s*error", stripped + + elif re.match(rgx_header_match.format(err_or_warn="error"), stripped): + curr_state = ReviewParseState.IN_ERROR + + elif re.match(rgx_header_match.format(err_or_warn="warning"), stripped): + curr_state = ReviewParseState.IN_WARNING + + elif curr_state == ReviewParseState.IN_ERROR and not re.match( + rgx_filler_match, stripped ): + curr_state = ReviewParseState.NORMAL has_errors = True - elif re.match(r"^(#{1,3}\s+)?(\*{0,2})warning", stripped) or re.match( - r"^<h[1-3]>\s*warning", stripped + + elif curr_state == ReviewParseState.IN_WARNING and not re.match( + rgx_filler_match, stripped ): + curr_state = ReviewParseState.NORMAL has_warnings = True + else: + curr_state = ReviewParseState.NORMAL + if has_errors: return EXIT_ERRORS if has_warnings: -- 2.54.0
