feat(review): walkthrough/risk_verdict/test_coverage schema
This commit is contained in:
@@ -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`,
|
`ruleId` examples: `QUALITY_DEAD_CODE`, `QUALITY_HIDDEN_COMPLEXITY`,
|
||||||
`QUALITY_INVARIANT_DROP`, `QUALITY_NAMING_CONTRADICTS`,
|
`QUALITY_INVARIANT_DROP`, `QUALITY_NAMING_CONTRADICTS`,
|
||||||
`QUALITY_SUPPRESSED_ERROR`, `QUALITY_DUPLICATED_LOGIC`. One stable
|
`QUALITY_SUPPRESSED_ERROR`, `QUALITY_DUPLICATED_LOGIC`. One stable
|
||||||
|
|||||||
@@ -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`,
|
`ruleId` examples: `DOCS_README_DRIFT`, `DOCS_FENCE_BROKEN`,
|
||||||
`DOCS_ENV_UNDOCUMENTED`, `DOCS_LINK_ROT`, `DOCS_NO_CHANGELOG`. Use one
|
`DOCS_ENV_UNDOCUMENTED`, `DOCS_LINK_ROT`, `DOCS_NO_CHANGELOG`. Use one
|
||||||
stable ruleId per recurring pattern — it's how the synthesizer dedups
|
stable ruleId per recurring pattern — it's how the synthesizer dedups
|
||||||
|
|||||||
@@ -48,3 +48,9 @@ O(n²) over bounded small n, `low` for redundant-but-rare work.
|
|||||||
```
|
```
|
||||||
|
|
||||||
`line` must be a post-change line. No prose outside JSON.
|
`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).
|
||||||
@@ -160,6 +160,12 @@ containing STRICT JSON, nothing else after it:
|
|||||||
"risks": [
|
"risks": [
|
||||||
"Bullets detailing potential bugs, edge cases, lifecycle issues, or performance risks found across the diff"
|
"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: <one-line concrete reason>",
|
||||||
|
"test_coverage": "Tests added" | "Tests changed" | "No tests for behavioral change" | "No test files in repo",
|
||||||
"findings": [
|
"findings": [
|
||||||
{
|
{
|
||||||
"severity": "critical|high|medium|low|info|nit",
|
"severity": "critical|high|medium|low|info|nit",
|
||||||
@@ -178,6 +184,16 @@ Rules:
|
|||||||
- `summary_changes` (2–4 bullets) goes into the **Summary of Changes** section.
|
- `summary_changes` (2–4 bullets) goes into the **Summary of Changes** section.
|
||||||
`risks` (bullets) goes into **Key Risks & Concerns**. Both are required;
|
`risks` (bullets) goes into **Key Risks & Concerns**. Both are required;
|
||||||
empty arrays are fine when nothing applies.
|
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 —
|
- `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 `""`
|
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).
|
when no safe textual replacement exists (e.g. missing test, architectural note).
|
||||||
|
|||||||
@@ -51,3 +51,9 @@ security findings only:
|
|||||||
|
|
||||||
`line` must be a post-change (context or `+`) line. Empty `suggestion` when no
|
`line` must be a post-change (context or `+`) line. Empty `suggestion` when no
|
||||||
safe replacement. No prose outside the JSON block.
|
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).
|
||||||
@@ -48,3 +48,9 @@ replacement); include a sketch only if a one-line test is obvious.
|
|||||||
```
|
```
|
||||||
|
|
||||||
`line` must be a post-change line in a source or test file. No prose outside JSON.
|
`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).
|
||||||
+40
-14
@@ -109,7 +109,10 @@ Output STRICT JSON only — no prose, no markdown fences. Shape:
|
|||||||
"fix": "one line: how to fix it",
|
"fix": "one line: how to fix it",
|
||||||
"suggestion": "<exact replacement lines for that location, or empty string if you cannot produce safe replacement code>"
|
"suggestion": "<exact replacement lines for that location, or empty string if you cannot produce safe replacement code>"
|
||||||
}
|
}
|
||||||
]
|
],
|
||||||
|
"walkthrough": ["2-6 short bullets, file- or change-grouped, plain prose"],
|
||||||
|
"risk_verdict": "Low|Medium|High|Critical risk: <one-line concrete reason>",
|
||||||
|
"test_coverage": "Tests added" | "Tests changed" | "No tests for behavioral change" | "No test files in repo"
|
||||||
}
|
}
|
||||||
|
|
||||||
Rules:
|
Rules:
|
||||||
@@ -121,6 +124,15 @@ Rules:
|
|||||||
Keep it minimal — just the changed lines, indented as they would appear in the
|
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.
|
file. Leave it empty ("") if a safe textual replacement is not possible (e.g.
|
||||||
a missing test, an architectural note).
|
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
|
- Skip nitpicks, pure formatting, and praise. At most ~15 findings, highest
|
||||||
severity first.
|
severity first.
|
||||||
- If the diff is clean, output: {"findings": []}
|
- 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]]:
|
def parse_review_output(
|
||||||
"""Parse the opengine's stdout into (summary, findings, summary_changes, risks).
|
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": [...],
|
Accepts `{"summary": "...", "summary_changes": [...], "risks": [...],
|
||||||
"findings": [...]}` (the opencode pragent agent), `{"findings": [...]}`,
|
"walkthrough": [...], "risk_verdict": "...", "test_coverage": "...",
|
||||||
or a bare `[...]` of finding dicts. `summary_changes` and `risks` default
|
"findings": [...]}` (the opencode pragent agent), the legacy 4-field
|
||||||
to empty lists; older outputs without them still parse fine. Uses the
|
shape, or a bare `[...]` of finding dicts. The three new fields
|
||||||
LAST fenced block (the pragent agent emits JSON as the final block), with
|
(`walkthrough`, `risk_verdict`, `test_coverage`) default to empty
|
||||||
a tolerant fallback that scans for the last balanced object/array in the
|
list / empty strings when absent — older outputs and the bare-array
|
||||||
prose tail. Never raises.
|
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)
|
blob = _last_json_block(text)
|
||||||
if blob is None:
|
if blob is None:
|
||||||
return "", [], [], []
|
return "", [], [], [], [], "", ""
|
||||||
try:
|
try:
|
||||||
data = json.loads(blob)
|
data = json.loads(blob)
|
||||||
except json.JSONDecodeError:
|
except json.JSONDecodeError:
|
||||||
return "", [], [], []
|
return "", [], [], [], [], "", ""
|
||||||
summary = ""
|
summary = ""
|
||||||
summary_changes: list[str] = []
|
summary_changes: list[str] = []
|
||||||
risks: list[str] = []
|
risks: list[str] = []
|
||||||
|
walkthrough: list[str] = []
|
||||||
|
risk_verdict = ""
|
||||||
|
test_coverage = ""
|
||||||
findings_raw = None
|
findings_raw = None
|
||||||
if isinstance(data, dict):
|
if isinstance(data, dict):
|
||||||
summary = str(data.get("summary", "") or "").strip()
|
summary = str(data.get("summary", "") or "").strip()
|
||||||
summary_changes = _string_list(data.get("summary_changes"))
|
summary_changes = _string_list(data.get("summary_changes"))
|
||||||
risks = _string_list(data.get("risks"))
|
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")
|
findings_raw = data.get("findings")
|
||||||
elif isinstance(data, list):
|
elif isinstance(data, list):
|
||||||
# Bare array: each item is a finding; no summary/sections.
|
# Bare array: each item is a finding; no summary/sections.
|
||||||
findings_raw = data
|
findings_raw = data
|
||||||
else:
|
else:
|
||||||
return "", [], [], []
|
return "", [], [], [], [], "", ""
|
||||||
out = []
|
out = []
|
||||||
if isinstance(findings_raw, list):
|
if isinstance(findings_raw, list):
|
||||||
for f in findings_raw:
|
for f in findings_raw:
|
||||||
n = _normalize_finding(f)
|
n = _normalize_finding(f)
|
||||||
if n is not None:
|
if n is not None:
|
||||||
out.append(n)
|
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]:
|
def _string_list(value) -> list[str]:
|
||||||
@@ -2028,7 +2054,7 @@ def review_pr(
|
|||||||
compression_note=compression_note,
|
compression_note=compression_note,
|
||||||
additional_context=additional_context,
|
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:
|
if not findings and not review_summary:
|
||||||
# The findings JSON was missing or malformed. Don't discard the
|
# The findings JSON was missing or malformed. Don't discard the
|
||||||
# run: salvage the prose, keep the usage report (the label asked
|
# run: salvage the prose, keep the usage report (the label asked
|
||||||
|
|||||||
@@ -905,6 +905,26 @@ def synthesize(
|
|||||||
return deduped[:per_pr_cap]
|
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
|
# 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("_")}
|
{k: v for k, v in f.items() if not k.startswith("_")}
|
||||||
for f in merged
|
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 = (
|
text = (
|
||||||
f"{summary}\n\n"
|
f"{summary}\n\n"
|
||||||
f"## Findings (multi-lens)\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:
|
if merged_usage is not None:
|
||||||
merged_usage["duration_s"] = round(time.monotonic() - t0, 1)
|
merged_usage["duration_s"] = round(time.monotonic() - t0, 1)
|
||||||
|
|||||||
@@ -479,9 +479,9 @@ def test_parse_review_output_bare_findings_no_summary():
|
|||||||
|
|
||||||
|
|
||||||
def test_parse_review_output_empty_and_bogus():
|
def test_parse_review_output_empty_and_bogus():
|
||||||
assert parse_review_output("") == ("", [], [], [])
|
assert parse_review_output("") == ("", [], [], [], [], "", "")
|
||||||
assert parse_review_output("no json here") == ("", [], [], [])
|
assert parse_review_output("no json here") == ("", [], [], [], [], "", "")
|
||||||
assert parse_review_output('{"findings":[]}') == ("", [], [], [])
|
assert parse_review_output('{"findings":[]}') == ("", [], [], [], [], "", "")
|
||||||
|
|
||||||
|
|
||||||
def test_parse_review_output_uses_last_json_block():
|
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
|
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():
|
def test_scan_balanced_handles_braces_in_strings():
|
||||||
# The JSON scanner must not be fooled by `{` or `}` inside string literals.
|
# The JSON scanner must not be fooled by `{` or `}` inside string literals.
|
||||||
s = '{"a":"contains { and }","b":1}'
|
s = '{"a":"contains { and }","b":1}'
|
||||||
|
|||||||
@@ -844,7 +844,9 @@ def test_no_surface_response_parses_as_an_empty_review():
|
|||||||
import ai_review
|
import ai_review
|
||||||
text, usage = oc._no_surface_response("o/r", "9", "abc12345", 3)
|
text, usage = oc._no_surface_response("o/r", "9", "abc12345", 3)
|
||||||
assert usage is None
|
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 findings == []
|
||||||
assert summary # non-empty, so ai_review does NOT take the salvage branch
|
assert summary # non-empty, so ai_review does NOT take the salvage branch
|
||||||
assert "no review surface" in summary.lower()
|
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():
|
def test_no_surface_response_zero_lenses_wording():
|
||||||
import ai_review
|
import ai_review
|
||||||
text, _ = oc._no_surface_response("o/r", "9", "abc12345", 0)
|
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 findings == []
|
||||||
assert "after path filtering" in summary
|
assert "after path filtering" in summary
|
||||||
|
|||||||
Reference in New Issue
Block a user