From e5a6e8923d527d71fbb29952eefcc455188b0696 Mon Sep 17 00:00:00 2001 From: claude Date: Sat, 22 Aug 2026 00:46:08 +0000 Subject: [PATCH] feat(opencode_review): python fallback for summary fields --- pilot/opencode_review.py | 79 +++++++++++++++++++++++++++-- tests/pilot/test_opencode_review.py | 46 +++++++++++++++++ 2 files changed, 120 insertions(+), 5 deletions(-) diff --git a/pilot/opencode_review.py b/pilot/opencode_review.py index d0003a8..f9ffc49 100644 --- a/pilot/opencode_review.py +++ b/pilot/opencode_review.py @@ -55,6 +55,8 @@ import time import urllib.error import urllib.request +from ai_review import is_test_path + # Where the factory lives (opencode.json + .opencode/). Default: the pragent # repo root (this file is at /pilot/opencode_review.py). _DEFAULT_FACTORY = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -565,6 +567,14 @@ _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: @@ -917,12 +927,69 @@ def _synthesize_summary_fields( (`ai_review.parse_review_output` extracts them as the 5th, 6th, and 7th tuple elements, defaulting to `[]` / `""` when missing). - STUB for Task 7. The real implementation arrives in Task 8; for now - every return is empty so the synthesized JSON shape stays parseable - and downstream tests that default the new fields to `[]` / `""` - continue to pass. + Real implementation (Task 8). Python fallback used when the lens + fan-out path is engaged (the synthesized JSON fence in `run_lenses_review` + has no model to call, so we build these fields deterministically from + the merged findings + the diff): + - walkthrough: one line per changed file. When findings exist, group + by path and pick the peak-severity problem as the headline; when + no findings exist, just announce "changed". + - risk_verdict: a one-line verdict driven by the highest severity + bucket that has any findings ("Critical risk" / "High risk" / + "Medium risk" / "Low risk"). + - test_coverage: "Tests changed" if any changed path matches + `is_test_path`, else "No tests for behavioral change in ``." + pointing at the first non-test path. """ - return [], "", "" + # walkthrough + walkthrough: list[str] = [] + if findings: + by_path: dict[str, list[dict]] = {} + for f in findings: + by_path.setdefault(f.get("path", "?"), []).append(f) + for path, group in sorted(by_path.items()): + peak = max( + group, + key=lambda x: SEVERITY_RANK.get(x.get("severity", "low"), 0), + ) + 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"), "⚪") + walkthrough.append(f"`{path}` — {emoji} {problem}") + else: + files = changed_paths if changed_paths is not None else changed_files(diff) + for p in files: + walkthrough.append(f"`{p}` — changed") + + # risk_verdict + sev_counts = {"critical": 0, "high": 0, "medium": 0, "low": 0} + for f in findings: + s = f.get("severity", "low") + sev_counts[s] = sev_counts.get(s, 0) + 1 + if sev_counts["critical"]: + rv = f"Critical risk: {sev_counts['critical']} critical finding(s)." + elif sev_counts["high"]: + rv = f"High risk: {sev_counts['high']} high finding(s)." + elif sev_counts["medium"]: + rv = f"Medium risk: {sev_counts['medium']} medium finding(s)." + else: + rv = "Low risk: clean or minor nits only." + + # test_coverage + paths = changed_paths if changed_paths is not None else changed_files(diff) + test_changed = any(is_test_path(p) for p in paths) + non_test = [p for p in paths if not is_test_path(p)] + if test_changed and non_test: + tc = "Tests changed" + elif non_test: + tc = f"No tests for behavioral change in `{non_test[0]}`." + elif test_changed: + tc = "Tests changed" + else: + tc = "" + + return walkthrough, rv, tc # --------------------------------------------------------------------------- @@ -1334,6 +1401,8 @@ def run_lenses_review( ) synthesized_payload = { "summary": summary, + "summary_changes": [], + "risks": [], "walkthrough": walkthrough, "risk_verdict": risk_verdict, "test_coverage": test_coverage, diff --git a/tests/pilot/test_opencode_review.py b/tests/pilot/test_opencode_review.py index 7db9e54..2e833b0 100644 --- a/tests/pilot/test_opencode_review.py +++ b/tests/pilot/test_opencode_review.py @@ -859,3 +859,49 @@ def test_no_surface_response_zero_lenses_wording(): summary, findings, _c, _r, _w, _rv, _tc = ai_review.parse_review_output(text) assert findings == [] assert "after path filtering" in summary + + +# --------------------------------------------------------------------------- +# _synthesize_summary_fields — Task 8: real Python fallback implementation +# --------------------------------------------------------------------------- + + +def test_synthesize_walkthrough_groups_findings_by_path(): + findings = [ + {"path": "a.py", "line": 1, "severity": "medium", "problem": "fix x"}, + {"path": "b.py", "line": 2, "severity": "high", "problem": "fix y"}, + ] + w, _, _ = oc._synthesize_summary_fields(findings, "") + assert any("a.py" in line for line in w) + assert any("b.py" in line for line in w) + + +def test_synthesize_walkthrough_empty_when_no_findings_uses_changed_files(): + w, _, _ = oc._synthesize_summary_fields( + [], + "diff --git a/x.py b/x.py\n@@ -1 +1 @@\n-old\n+new\n+++ b/x.py\n", + ) + assert any("x.py" in line for line in w) + + +def test_synthesize_risk_verdict_critical(): + findings = [{"severity": "critical"}] + _, rv, _ = oc._synthesize_summary_fields(findings, "") + assert "Critical risk" in rv + + +def test_synthesize_risk_verdict_clean(): + _, rv, _ = oc._synthesize_summary_fields([], "") + assert "Low risk" in rv + + +def test_synthesize_test_coverage_with_test_path(): + _, _, tc = oc._synthesize_summary_fields( + [], "+diff\n", changed_paths=["pilot/foo.py", "tests/test_foo.py"]) + assert tc == "Tests changed" + + +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