[PATCH v2 2/2] devtools/ai/review-patch: reduce spurious warnings
Stephen Hemminger
stephen at networkplumber.org
Fri Sep 4 18:23:08 CEST 2026
Improve the logic for reporting warnings and errors to
avoid flagging good patches as suspicious.
Signed-off-by: Stephen Hemminger <stephen at 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
More information about the dev
mailing list