diff --git a/pilot/opencode_review.py b/pilot/opencode_review.py index f9ffc49..c2524ab 100644 --- a/pilot/opencode_review.py +++ b/pilot/opencode_review.py @@ -55,7 +55,7 @@ import time import urllib.error import urllib.request -from ai_review import is_test_path +from ai_review import _SEVERITY_EMOJI, is_test_path # Where the factory lives (opencode.json + .opencode/). Default: the pragent # repo root (this file is at /pilot/opencode_review.py). @@ -567,14 +567,6 @@ _LENS_ID_RE = re.compile(r"^[a-z0-9-]{1,32}$") SEVERITY_ORDER = ("low", "medium", "high", "critical") SEVERITY_RANK = {s: i for i, s in enumerate(SEVERITY_ORDER)} -# Local emoji map for the synthesized walkthrough. Kept separate from -# `ai_review._SEVERITY_EMOJI` (which also has "trivial"/"info"/"nit") so -# this stays a small surface for the fallback path. -_SEVERITY_EMOJI_SUMMARY = { - "critical": "🔴", "high": "🔴", "medium": "🟡", - "low": "🔵", "trivial": "⚪", "info": "⚪", -} - @_dc.dataclass(frozen=True) class ReviewerSpec: @@ -941,6 +933,10 @@ def _synthesize_summary_fields( `is_test_path`, else "No tests for behavioral change in ``." pointing at the first non-test path. """ + # None-safe: callers occasionally pass None when the upstream merger + # short-circuited. Treat as empty so the for-loop and group-by below + # never crash. + findings = findings or [] # walkthrough walkthrough: list[str] = [] if findings: @@ -954,8 +950,7 @@ def _synthesize_summary_fields( ) problem_lines = (peak.get("problem") or "").splitlines() problem = problem_lines[0][:80].strip() if problem_lines else "" - emoji = _SEVERITY_EMOJI_SUMMARY.get( - peak.get("severity", "low"), "⚪") + emoji = _SEVERITY_EMOJI.get(peak.get("severity", "low"), "⚪") walkthrough.append(f"`{path}` — {emoji} {problem}") else: files = changed_paths if changed_paths is not None else changed_files(diff) diff --git a/tests/pilot/test_opencode_review.py b/tests/pilot/test_opencode_review.py index 2e833b0..1d4f4f0 100644 --- a/tests/pilot/test_opencode_review.py +++ b/tests/pilot/test_opencode_review.py @@ -905,3 +905,47 @@ def test_synthesize_test_coverage_missing_tests(): _, _, tc = oc._synthesize_summary_fields( [], "+diff\n", changed_paths=["pilot/foo.py"]) assert "No tests for behavioral change" in tc + + +def test_synthesize_walkthrough_picks_peak_severity_per_path(): + # Three findings on the same path, with mixed severities. The walkthrough + # headline should use the PEAK severity's emoji (critical = 🔴), not the + # lexicographic-first severity (low). + findings = [ + {"path": "x.py", "line": 1, "severity": "low", + "problem": "minor nit"}, + {"path": "x.py", "line": 5, "severity": "critical", + "problem": "sql injection"}, + {"path": "x.py", "line": 9, "severity": "high", + "problem": "auth bypass"}, + ] + w, _, _ = oc._synthesize_summary_fields(findings, "") + assert len(w) == 1 + line = w[0] + assert "`x.py`" in line + assert "🔴" in line # critical = 🔴 + assert "🟡" not in line + assert "🔵" not in line + assert "sql injection" in line # critical finding's problem, not low's + + +def test_synthesize_summary_fields_none_findings_safe(): + # Old code crashed in risk_verdict with `for f in findings:` on None. + # After the `findings = findings or []` guard, None behaves like []. + w, rv, tc = oc._synthesize_summary_fields(None, "") + assert isinstance(w, list) + assert rv.startswith("Low risk") + # walkthrough should fall through to the diff-derived path list — empty + # diff produces no lines, but no crash is the point. + assert tc == "" + + +def test_synthesize_walkthrough_empty_problem_does_not_crash(): + # An empty `problem` should render as "`a.py` — emoji" with a trailing + # space, not raise. Regression guard for splitlines()[0][:80].strip(). + findings = [{"path": "a.py", "line": 1, + "severity": "low", "problem": ""}] + w, _, _ = oc._synthesize_summary_fields(findings, "") + assert len(w) == 1 + assert "`a.py`" in w[0] + assert "🔵" in w[0] # low severity emoji