From 2e982846d96f9637f10c9e7b48b593c017e78ec4 Mon Sep 17 00:00:00 2001 From: claude Date: Sat, 22 Aug 2026 00:36:50 +0000 Subject: [PATCH] feat(review): walkthrough/risk_verdict/test_coverage schema --- .opencode/agents/code-quality.md | 6 ++++ .opencode/agents/docs.md | 6 ++++ .opencode/agents/perf.md | 8 ++++- .opencode/agents/pragent.md | 16 +++++++++ .opencode/agents/security.md | 8 ++++- .opencode/agents/tests.md | 8 ++++- pilot/ai_review.py | 54 +++++++++++++++++++++-------- pilot/opencode_review.py | 37 +++++++++++++++++++- tests/pilot/test_ai_review.py | 44 +++++++++++++++++++++-- tests/pilot/test_opencode_review.py | 6 ++-- 10 files changed, 170 insertions(+), 23 deletions(-) diff --git a/.opencode/agents/code-quality.md b/.opencode/agents/code-quality.md index 1e88a5e..e864807 100644 --- a/.opencode/agents/code-quality.md +++ b/.opencode/agents/code-quality.md @@ -68,6 +68,12 @@ Return STRICT JSON only — same shape as the pragent primary's findings: } ``` +The full review-level JSON shape (used by the pragent primary) also +includes three optional top-level fields — `walkthrough` (list[str]), +`risk_verdict` (str), and `test_coverage` (str) — that the synthesizer +fills in across all lenses. Lens output is free to omit them; the parser +defaults to `[]` / `""` when absent (backward compatible). + `ruleId` examples: `QUALITY_DEAD_CODE`, `QUALITY_HIDDEN_COMPLEXITY`, `QUALITY_INVARIANT_DROP`, `QUALITY_NAMING_CONTRADICTS`, `QUALITY_SUPPRESSED_ERROR`, `QUALITY_DUPLICATED_LOGIC`. One stable diff --git a/.opencode/agents/docs.md b/.opencode/agents/docs.md index 6561247..18c22da 100644 --- a/.opencode/agents/docs.md +++ b/.opencode/agents/docs.md @@ -67,6 +67,12 @@ Return STRICT JSON only — same shape as the pragent primary's findings: } ``` +The full review-level JSON shape (used by the pragent primary) also +includes three optional top-level fields — `walkthrough` (list[str]), +`risk_verdict` (str), and `test_coverage` (str) — that the synthesizer +fills in across all lenses. Lens output is free to omit them; the parser +defaults to `[]` / `""` when absent (backward compatible). + `ruleId` examples: `DOCS_README_DRIFT`, `DOCS_FENCE_BROKEN`, `DOCS_ENV_UNDOCUMENTED`, `DOCS_LINK_ROT`, `DOCS_NO_CHANGELOG`. Use one stable ruleId per recurring pattern — it's how the synthesizer dedups diff --git a/.opencode/agents/perf.md b/.opencode/agents/perf.md index cf9ff0b..a4098f7 100644 --- a/.opencode/agents/perf.md +++ b/.opencode/agents/perf.md @@ -47,4 +47,10 @@ O(n²) over bounded small n, `low` for redundant-but-rare work. {"findings":[{"severity":"...","path":"...","line":0,"problem":"...","fix":"...","suggestion":"","reference":""}]} ``` -`line` must be a post-change line. No prose outside JSON. \ No newline at end of file +`line` must be a post-change line. No prose outside JSON. + +The full review-level JSON shape (used by the pragent primary) also +includes three optional top-level fields — `walkthrough` (list[str]), +`risk_verdict` (str), and `test_coverage` (str) — that the synthesizer +fills in across all lenses. Lens output is free to omit them; the parser +defaults to `[]` / `""` when absent (backward compatible). \ No newline at end of file diff --git a/.opencode/agents/pragent.md b/.opencode/agents/pragent.md index 6b33752..176ea59 100644 --- a/.opencode/agents/pragent.md +++ b/.opencode/agents/pragent.md @@ -160,6 +160,12 @@ containing STRICT JSON, nothing else after it: "risks": [ "Bullets detailing potential bugs, edge cases, lifecycle issues, or performance risks found across the diff" ], + "walkthrough": [ + "a.py: adds X — short plain-prose bullet, file- or change-grouped", + "b.py: refactors Y" + ], + "risk_verdict": "Low|Medium|High|Critical risk: ", + "test_coverage": "Tests added" | "Tests changed" | "No tests for behavioral change" | "No test files in repo", "findings": [ { "severity": "critical|high|medium|low|info|nit", @@ -178,6 +184,16 @@ Rules: - `summary_changes` (2–4 bullets) goes into the **Summary of Changes** section. `risks` (bullets) goes into **Key Risks & Concerns**. Both are required; empty arrays are fine when nothing applies. +- `walkthrough` (2–6 bullets, file- or change-grouped) is the **Walkthrough** + section: what the PR does, where, in plain prose. Default to `[]` for a + trivial diff. Backward compatible — parsers default to `[]` if absent. +- `risk_verdict` (exactly one line) goes into the **Risk Verdict** section. + Lead with `Low|Medium|High|Critical risk:` followed by a concrete reason. + Default to `""` when not applicable. Backward compatible. +- `test_coverage` (short string) goes into the **Test Coverage** section. + Use exactly one of `"Tests added"`, `"Tests changed"`, + `"No tests for behavioral change"`, `"No test files in repo"`. Default to `""`. + Backward compatible. - `suggestion` is the literal new code that replaces the flagged line(s). Minimal — just the changed lines, indented as they'd appear in the file. Empty string `""` when no safe textual replacement exists (e.g. missing test, architectural note). diff --git a/.opencode/agents/security.md b/.opencode/agents/security.md index 5319a3c..7d01068 100644 --- a/.opencode/agents/security.md +++ b/.opencode/agents/security.md @@ -50,4 +50,10 @@ security findings only: ``` `line` must be a post-change (context or `+`) line. Empty `suggestion` when no -safe replacement. No prose outside the JSON block. \ No newline at end of file +safe replacement. No prose outside the JSON block. + +The full review-level JSON shape (used by the pragent primary) also +includes three optional top-level fields — `walkthrough` (list[str]), +`risk_verdict` (str), and `test_coverage` (str) — that the synthesizer +fills in across all lenses. Lens output is free to omit them; the parser +defaults to `[]` / `""` when absent (backward compatible). \ No newline at end of file diff --git a/.opencode/agents/tests.md b/.opencode/agents/tests.md index 5ae3e2f..5ac4e05 100644 --- a/.opencode/agents/tests.md +++ b/.opencode/agents/tests.md @@ -47,4 +47,10 @@ replacement); include a sketch only if a one-line test is obvious. {"findings":[{"severity":"...","path":"...","line":0,"problem":"...","fix":"...","suggestion":"","reference":""}]} ``` -`line` must be a post-change line in a source or test file. No prose outside JSON. \ No newline at end of file +`line` must be a post-change line in a source or test file. No prose outside JSON. + +The full review-level JSON shape (used by the pragent primary) also +includes three optional top-level fields — `walkthrough` (list[str]), +`risk_verdict` (str), and `test_coverage` (str) — that the synthesizer +fills in across all lenses. Lens output is free to omit them; the parser +defaults to `[]` / `""` when absent (backward compatible). \ No newline at end of file diff --git a/pilot/ai_review.py b/pilot/ai_review.py index 7a2a3b4..d720a07 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -109,7 +109,10 @@ Output STRICT JSON only — no prose, no markdown fences. Shape: "fix": "one line: how to fix it", "suggestion": "" } - ] + ], + "walkthrough": ["2-6 short bullets, file- or change-grouped, plain prose"], + "risk_verdict": "Low|Medium|High|Critical risk: ", + "test_coverage": "Tests added" | "Tests changed" | "No tests for behavioral change" | "No test files in repo" } Rules: @@ -121,6 +124,15 @@ Rules: Keep it minimal — just the changed lines, indented as they would appear in the file. Leave it empty ("") if a safe textual replacement is not possible (e.g. a missing test, an architectural note). +- `walkthrough`: 2-6 short bullets, file- or change-grouped, plain prose. + Default to `[]` when the diff is trivial. Backward compatible: parsers + default to `[]` if absent. +- `risk_verdict`: exactly one line. Lead with "Low|Medium|High|Critical risk:" + followed by a concrete reason. Default to `""` when not applicable. + Backward compatible: parsers default to `""` if absent. +- `test_coverage`: short string. One of "Tests added" / "Tests changed" / + "No tests for behavioral change" / "No test files in repo". Default to `""` + when not applicable. Backward compatible: parsers default to `""` if absent. - Skip nitpicks, pure formatting, and praise. At most ~15 findings, highest severity first. - If the diff is clean, output: {"findings": []} @@ -728,45 +740,59 @@ def salvage_summary(text: str, max_chars: int = SALVAGE_MAX_CHARS) -> str: ) -def parse_review_output(text: str) -> tuple[str, list[dict], list[str], list[str]]: - """Parse the opengine's stdout into (summary, findings, summary_changes, risks). +def parse_review_output( + text: str, +) -> tuple[str, list[dict], list[str], list[str], list[str], str, str]: + """Parse the opengine's stdout into a 7-tuple: + (summary, findings, summary_changes, risks, + walkthrough, risk_verdict, test_coverage) Accepts `{"summary": "...", "summary_changes": [...], "risks": [...], - "findings": [...]}` (the opencode pragent agent), `{"findings": [...]}`, - or a bare `[...]` of finding dicts. `summary_changes` and `risks` default - to empty lists; older outputs without them still parse fine. Uses the - LAST fenced block (the pragent agent emits JSON as the final block), with - a tolerant fallback that scans for the last balanced object/array in the - prose tail. Never raises. + "walkthrough": [...], "risk_verdict": "...", "test_coverage": "...", + "findings": [...]}` (the opencode pragent agent), the legacy 4-field + shape, or a bare `[...]` of finding dicts. The three new fields + (`walkthrough`, `risk_verdict`, `test_coverage`) default to empty + list / empty strings when absent — older outputs and the bare-array + shape stay backward compatible. + + Uses the LAST fenced block (the pragent agent emits JSON as the final + block), with a tolerant fallback that scans for the last balanced + object/array in the prose tail. Never raises. """ blob = _last_json_block(text) if blob is None: - return "", [], [], [] + return "", [], [], [], [], "", "" try: data = json.loads(blob) except json.JSONDecodeError: - return "", [], [], [] + return "", [], [], [], [], "", "" summary = "" summary_changes: list[str] = [] risks: list[str] = [] + walkthrough: list[str] = [] + risk_verdict = "" + test_coverage = "" findings_raw = None if isinstance(data, dict): summary = str(data.get("summary", "") or "").strip() summary_changes = _string_list(data.get("summary_changes")) risks = _string_list(data.get("risks")) + walkthrough = _string_list(data.get("walkthrough")) + risk_verdict = str(data.get("risk_verdict", "") or "").strip() + test_coverage = str(data.get("test_coverage", "") or "").strip() findings_raw = data.get("findings") elif isinstance(data, list): # Bare array: each item is a finding; no summary/sections. findings_raw = data else: - return "", [], [], [] + return "", [], [], [], [], "", "" out = [] if isinstance(findings_raw, list): for f in findings_raw: n = _normalize_finding(f) if n is not None: out.append(n) - return summary, out, summary_changes, risks + return summary, out, summary_changes, risks, walkthrough, risk_verdict, test_coverage def _string_list(value) -> list[str]: @@ -2028,7 +2054,7 @@ def review_pr( compression_note=compression_note, additional_context=additional_context, ) - review_summary, findings, summary_changes, risks = parse_review_output(stdout) + review_summary, findings, summary_changes, risks, _walkthrough, _risk_verdict, _test_coverage = parse_review_output(stdout) if not findings and not review_summary: # The findings JSON was missing or malformed. Don't discard the # run: salvage the prose, keep the usage report (the label asked diff --git a/pilot/opencode_review.py b/pilot/opencode_review.py index 299eb5d..d0003a8 100644 --- a/pilot/opencode_review.py +++ b/pilot/opencode_review.py @@ -905,6 +905,26 @@ def synthesize( return deduped[:per_pr_cap] +def _synthesize_summary_fields( + findings: list[dict], + diff: str, + changed_paths: list[str] | None = None, +) -> tuple[list[str], str, str]: + """Synthesize review-level meta from the merged findings + diff. + + Returns (walkthrough, risk_verdict, test_coverage) — the three new + top-level fields in the pragent review JSON shape + (`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. + """ + return [], "", "" + + # --------------------------------------------------------------------------- # Per-lens subprocess + parallel fan-out # --------------------------------------------------------------------------- @@ -1304,10 +1324,25 @@ def run_lenses_review( {k: v for k, v in f.items() if not k.startswith("_")} for f in merged ] + # Synthesize the review-level meta (walkthrough / risk_verdict / + # test_coverage) from the merged findings + diff. Real implementation + # arrives in Task 8; the stub keeps the synthesized JSON shape stable + # so ai_review.parse_review_output can extract the three new fields + # (it defaults them to [] / "" when missing — backward compatible). + walkthrough, risk_verdict, test_coverage = _synthesize_summary_fields( + merged, diff, changed_paths=changed_paths, + ) + synthesized_payload = { + "summary": summary, + "walkthrough": walkthrough, + "risk_verdict": risk_verdict, + "test_coverage": test_coverage, + "findings": clean_findings, + } text = ( f"{summary}\n\n" f"## Findings (multi-lens)\n\n" - f"```json\n{json.dumps({'summary': summary, 'findings': clean_findings}, indent=2)}\n```\n" + f"```json\n{json.dumps(synthesized_payload, indent=2)}\n```\n" ) if merged_usage is not None: merged_usage["duration_s"] = round(time.monotonic() - t0, 1) diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index e608e16..31c8f6a 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -479,9 +479,9 @@ def test_parse_review_output_bare_findings_no_summary(): def test_parse_review_output_empty_and_bogus(): - assert parse_review_output("") == ("", [], [], []) - assert parse_review_output("no json here") == ("", [], [], []) - assert parse_review_output('{"findings":[]}') == ("", [], [], []) + assert parse_review_output("") == ("", [], [], [], [], "", "") + assert parse_review_output("no json here") == ("", [], [], [], [], "", "") + assert parse_review_output('{"findings":[]}') == ("", [], [], [], [], "", "") def test_parse_review_output_uses_last_json_block(): @@ -565,6 +565,44 @@ def test_parse_review_output_bare_array_at_tail(): assert len(fs) == 1 +def test_parse_review_output_extracts_walkthrough_risk_tests(): + # The 7-tuple shape carries three new top-level fields: + # walkthrough (list[str]), risk_verdict (str), test_coverage (str). + txt = ( + "```json\n" + "{\n" + ' "summary": "x",\n' + ' "summary_changes": [],\n' + ' "risks": [],\n' + ' "walkthrough": ["a.py: adds X", "b.py: refactors Y"],\n' + ' "risk_verdict": "Low risk.",\n' + ' "test_coverage": "No tests for behavioral change in a.py.",\n' + ' "findings": []\n' + "}\n" + "```" + ) + summary, findings, _changes, _risks, walkthrough, risk_verdict, test_coverage = ( + parse_review_output(txt) + ) + assert summary == "x" + assert findings == [] + assert walkthrough == ["a.py: adds X", "b.py: refactors Y"] + assert risk_verdict == "Low risk." + assert test_coverage == "No tests for behavioral change in a.py." + + +def test_parse_review_output_missing_fields_default_empty(): + # Backward-compatible: the 4-tuple shape still parses fine; the new + # fields default to empty list / empty string. + out = parse_review_output('{"summary":"x","findings":[]}') + summary, findings, _changes, _risks, walkthrough, risk_verdict, test_coverage = out + assert summary == "x" + assert findings == [] + assert walkthrough == [] + assert risk_verdict == "" + assert test_coverage == "" + + def test_scan_balanced_handles_braces_in_strings(): # The JSON scanner must not be fooled by `{` or `}` inside string literals. s = '{"a":"contains { and }","b":1}' diff --git a/tests/pilot/test_opencode_review.py b/tests/pilot/test_opencode_review.py index 1209b24..7db9e54 100644 --- a/tests/pilot/test_opencode_review.py +++ b/tests/pilot/test_opencode_review.py @@ -844,7 +844,9 @@ def test_no_surface_response_parses_as_an_empty_review(): import ai_review text, usage = oc._no_surface_response("o/r", "9", "abc12345", 3) assert usage is None - summary, findings, _changes, _risks = ai_review.parse_review_output(text) + summary, findings, _changes, _risks, _walkthrough, _risk_verdict, _test_coverage = ( + ai_review.parse_review_output(text) + ) assert findings == [] assert summary # non-empty, so ai_review does NOT take the salvage branch assert "no review surface" in summary.lower() @@ -854,6 +856,6 @@ def test_no_surface_response_parses_as_an_empty_review(): def test_no_surface_response_zero_lenses_wording(): import ai_review text, _ = oc._no_surface_response("o/r", "9", "abc12345", 0) - summary, findings, _c, _r = ai_review.parse_review_output(text) + summary, findings, _c, _r, _w, _rv, _tc = ai_review.parse_review_output(text) assert findings == [] assert "after path filtering" in summary