[PATCH v2 2/2] devtools/ai/review-patch: reduce spurious warnings

[Stephen Hemminger <stephen@networkplumber.org>]

Improve the logic for reporting warnings and errors to
avoid flagging good patches as suspicious.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 devtools/ai/review-patch.py | 232 +++++++++++++++++++++++++++++-------
 1 file changed, 186 insertions(+), 46 deletions(-)

diff --git a/devtools/ai/review-patch.py b/devtools/ai/review-patch.py
index 5f8d9ed772..9b5b36f40d 100755
--- a/devtools/ai/review-patch.py
+++ b/devtools/ai/review-patch.py
@@ -19,7 +19,6 @@
 from email.message import EmailMessage
 from pathlib import Path
 from typing import Any, Iterator
-from enum import Flag, auto
 
 from _common import (
     PROVIDERS,
@@ -69,19 +68,31 @@
 - Be conservative: reject changes that aren't clearly bug fixes"""
 
 FORMAT_INSTRUCTIONS = {
-    "text": """Provide your review in plain text format.""",
+    "text": """Provide your review in plain text format.
+
+End the review with a single final line, exactly:
+Review-Result: CLEAN|WARNING|ERROR
+CLEAN means no Errors and no Warnings. This line is parsed by CI.""",
     "markdown": """Provide your review in Markdown format with:
 - Headers (##) for each severity level (Errors, Warnings, Info)
 - Bullet points for individual issues
 - Code blocks (```) for code references
-- Bold (**) for emphasis on key points""",
+- Bold (**) for emphasis on key points
+
+End the review with a single final line, exactly:
+Review-Result: CLEAN|WARNING|ERROR
+CLEAN means no Errors and no Warnings. This line is parsed by CI.""",
     "html": """Provide your review in HTML format with:
 - <h2> tags for each severity level (Errors, Warnings, Info)
 - <ul>/<li> for individual issues
 - <pre><code> for code references
 - <strong> for emphasis on key points
 - Use appropriate semantic HTML tags
-- Do NOT include <html>, <head>, or <body> tags - just the content""",
+- Do NOT include <html>, <head>, or <body> tags - just the content
+
+End the review with a single final line, exactly:
+Review-Result: CLEAN|WARNING|ERROR
+CLEAN means no Errors and no Warnings. This line is parsed by CI.""",
     "json": """Provide your review in JSON format with this structure:
 {
   "summary": "Brief one-line summary of the review",
@@ -122,10 +133,134 @@
 EXIT_ERRORS = 3
 
 
-class ReviewParseState(Flag):
-    NORMAL = auto()
-    IN_ERROR = auto()
-    IN_WARNING = auto()
+# Explicit machine-readable verdict; authoritative when present.
+# Quoted lines ('>') are excluded: a patch that adds a Review-Result line to a
+# document or test fixture must not be able to dictate its own verdict.
+_RESULT_RE = re.compile(
+    r"^[\s*#-]*review-result:\s*[*`\s]*(clean|warning|error)s?\b", re.MULTILINE
+)
+_RESULT_EXIT = {"clean": EXIT_CLEAN, "warning": EXIT_WARNINGS, "error": EXIT_ERRORS}
+
+# A severity section header: the whole line is the word plus decoration.
+#   "## Errors"  "**Warnings**"  "<h2>Errors</h2>"  "ERRORS:"  "Error (must fix):"
+_HEADER_RE = re.compile(
+    r"^(?:#{1,6}\s*)?(?:<h[1-6]>\s*)?[*_`]{0,2}"
+    r"(error|warning)s?"
+    r"[*_`]{0,2}\s*(?:\((?:must|should) fix\))?\s*:?\s*(?:</h[1-6]>)?$"
+)
+
+# An inline finding marker: "Error: foo", "- **Warning** - bar".
+# The remainder is captured so that "Errors: none" is recognised as filler
+# rather than counted as a finding.
+_INLINE_RE = re.compile(
+    r"^[-*\s]*[*_`]{0,2}(error|warning)s?[*_`]{0,2}\s*[:\u2013-]\s*(\S.*)$"
+)
+
+# Text meaning "this section is empty". Deliberately a closed vocabulary:
+# anything else under a severity header counts as a finding.
+_EMPTY_RE = re.compile(
+    r"^[-*\s]*[*_`(\[]*\s*"
+    r"(?:none|nil|n/?a"
+    r"|no\s+(?:errors?|warnings?|issues?|problems?|findings?|concerns?"
+    r"|correctness\s+bugs?|changes?\s+(?:required|needed)))"
+    r"[\w\s]{0,24}[.!]?\s*[*_`)\]]*$"
+)
+
+_RULE_CHARS = set("-=_*# \t")
+
+# Code block delimiters. The markdown instructions ask for code references, so a
+# quoted compiler diagnostic or RTE_LOG(WARNING, ...) must not read as a finding.
+_FENCE_RE = re.compile(r"^(?:```|~~~)")
+
+
+def _asserted_lines(review_text: str) -> Iterator[str]:
+    """Yield the lines a review asserts, dropping the ones it quotes.
+
+    Code fences and '>' context carry text the review is talking *about*
+    rather than claiming, so neither may open a section or supply a verdict.
+    Indentation is left to the caller: an indented line may be a code sample,
+    but it may equally be a finding nested under a severity header.
+    """
+    in_fence = False
+
+    for line in review_text.splitlines():
+        if _FENCE_RE.match(line.strip()):
+            in_fence = not in_fence
+            continue
+        if in_fence:
+            continue
+        if line.lstrip().startswith(">"):
+            continue
+        yield line
+
+
+def strip_quoted_blocks(review_text: str) -> str:
+    """Return the text that may supply a verdict line.
+
+    Indented blocks are dropped here: a review that shows an example
+    Review-Result line must not be able to dictate its own verdict.
+    """
+    return "\n".join(
+        line
+        for line in _asserted_lines(review_text)
+        if not line.startswith("    ") and not line.startswith("\t")
+    )
+
+
+def scan_review_prose(review_text: str) -> tuple[bool, bool]:
+    """Scan free-form review text for findings.
+
+    A severity header only counts if the section under it holds something
+    other than "none". Returns (has_errors, has_warnings).
+    """
+    has_errors = False
+    has_warnings = False
+    section: str | None = None
+
+    for line in _asserted_lines(review_text):
+        stripped = line.strip().lower()
+
+        # Blank lines, horizontal rules and patch context are inert:
+        # they neither open nor close a section.
+        if (
+            not stripped
+            or set(stripped) <= _RULE_CHARS
+            or stripped.startswith("diff --git")
+        ):
+            continue
+
+        # An indented line may be an unfenced code sample, so it cannot open
+        # a section or stand alone as a finding.  Under an open header it is
+        # still content: a bullet nested under "## Errors" is a finding.
+        if not line.startswith("    ") and not line.startswith("\t"):
+            match = _INLINE_RE.match(stripped)
+            if match:
+                if not _EMPTY_RE.match(match.group(2)):
+                    if match.group(1) == "error":
+                        has_errors = True
+                    else:
+                        has_warnings = True
+                section = None
+                continue
+
+            match = _HEADER_RE.match(stripped)
+            if match:
+                section = match.group(1)
+                continue
+
+        if section is None:
+            continue
+
+        # Strip HTML tags so "<p>None identified.</p>" reads as filler.
+        text = re.sub(r"<[^>]+>", " ", stripped).strip()
+        if not _EMPTY_RE.match(text):
+            if section == "error":
+                has_errors = True
+            else:
+                has_warnings = True
+        section = None
+
+    return has_errors, has_warnings
 
 
 def classify_review(review_text: str, output_format: str) -> int:
@@ -136,12 +271,37 @@ def classify_review(review_text: str, output_format: str) -> int:
         2 - warnings found (no errors)
         3 - errors found
     """
+    # 1. Explicit verdict line wins. Combined reviews carry one per section;
+    #    the worst result decides. Quoted material cannot supply one.
+    verdicts = _RESULT_RE.findall(strip_quoted_blocks(review_text).lower())
+    if verdicts:
+        return max(_RESULT_EXIT[v] for v in verdicts)
+
     has_errors = False
     has_warnings = False
 
+    # 2. Structured JSON.
     if output_format == "json":
         try:
             data = json.loads(review_text)
+            # Combined reviews nest one sub-review per patch or chunk, each
+            # still in its own format. Classify each and keep the worst.
+            sections = data.get("sections")
+            if isinstance(sections, list):
+                worst = EXIT_CLEAN
+                classified = False
+                for entry in sections:
+                    if not isinstance(entry, dict):
+                        continue
+                    review = entry.get("review")
+                    if isinstance(review, str):
+                        classified = True
+                        worst = max(worst, classify_review(review, output_format))
+                # An empty or unrecognised "sections" value is not a verdict;
+                # fall through to the top-level keys rather than report clean.
+                if classified:
+                    return worst
+
             if data.get("errors"):
                 has_errors = True
             if data.get("warnings"):
@@ -154,44 +314,9 @@ def classify_review(review_text: str, output_format: str) -> int:
         except (json.JSONDecodeError, AttributeError):
             pass  # Fall through to text scanning
 
+    # 3. Fallback: scan prose.
     if not has_errors and not has_warnings:
-        # 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_state: ReviewParseState = ReviewParseState.NORMAL
-
-        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
-
-            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 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
+        has_errors, has_warnings = scan_review_prose(review_text)
 
     if has_errors:
         return EXIT_ERRORS
@@ -1164,7 +1289,10 @@ def main() -> None:
     if args.verbose:
         print("=== Request ===", file=sys.stderr)
         print(f"Provider: {args.provider}", file=sys.stderr)
-        print(f"Auth method: {'vertex' if auth == 'vertex' else 'direct'}", file=sys.stderr)
+        print(
+            f"Auth method: {'vertex' if auth == 'vertex' else 'direct'}",
+            file=sys.stderr,
+        )
         print(f"Model: {model}", file=sys.stderr)
         print(f"Review date: {review_date}", file=sys.stderr)
         if args.release:
@@ -1351,6 +1479,18 @@ def main() -> None:
             print("", file=sys.stderr)
             print(f"Review sent to: {', '.join(args.to_addrs)}", file=sys.stderr)
 
+    # The format instructions require a final Review-Result line.  Without it
+    # the severity below is inferred from prose, which is a guess -- and the
+    # usual cause is a response truncated before the review was finished.
+    if args.output_format != "json" and not _RESULT_RE.search(
+        strip_quoted_blocks(review_text).lower()
+    ):
+        print(
+            "warning: review has no Review-Result line, "
+            "severity inferred from prose (response may be truncated)",
+            file=sys.stderr,
+        )
+
     # Exit with code based on review severity
     sys.exit(classify_review(review_text, args.output_format))
 
-- 
2.53.0