feat(review): PR-level collapsible metadata + emoji-tagged inline comments
PR-level comment layout (per operator's format guide):
* Summary of Changes — 2-4 bullets, sourced from the agent's new
`summary_changes` JSON field. Falls back to splitting the prose
`summary` if the list is missing.
* Key Risks & Concerns — bullets from the new `risks` JSON field.
* Findings Overview — Markdown table covering every finding
(severity emoji / location / one-line problem). Both anchored and
unanchored findings appear here so the table is the single scan point.
* Unanchored Notes — bullets with severity + fix + Markdown-linked ref,
for findings with no post-change line to anchor.
* AI Usage & Run Details — wrapped in a <details>/<summary> collapsible
so the body stays scannable. Cost line stays inside it.
Inline comment shape:
* Severity badge: 🔴 [HIGH] / 🟡 [MEDIUM] / 🔵 [LOW] / ⚪ [INFO].
Unknown severities fall back to � [INFO].
* 1-2 short paragraphs of problem; **Fix:** label for the fix line.
* Standard ```suggestion fence for replacement code (Gitea/Forgejo
apply-on-click). Language-tagged fences are no longer used for
single-file diffs.
* Reference as a Markdown hyperlink, visible label truncated to
<=60 chars; the underlying URL is preserved verbatim.
* NO per-comment 🪙 token attribution. All telemetry stays in the
collapsible block on the PR-level comment.
Agent prompt updated to emit `summary_changes` and `risks` in the JSON
output (backward-compatible — older outputs missing them still parse;
they fall back to splitting the prose `summary`).
Tests: 15 new (severity emoji mapping, reference truncation, findings
table escaping, collapsible usage rendering, summary_changes+risks
layout). Existing tests updated for the new structure.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -142,12 +142,18 @@ containing STRICT JSON, nothing else after it:
|
||||
```json
|
||||
{
|
||||
"summary": "One-paragraph overview of the change and its risk.",
|
||||
"summary_changes": [
|
||||
"2–4 short bullets explaining what the PR introduces or modifies"
|
||||
],
|
||||
"risks": [
|
||||
"Bullets detailing potential bugs, edge cases, lifecycle issues, or performance risks found across the diff"
|
||||
],
|
||||
"findings": [
|
||||
{
|
||||
"severity": "critical|high|medium|low",
|
||||
"severity": "critical|high|medium|low|info|nit",
|
||||
"path": "path exactly as in the diff `+++ b/` side",
|
||||
"line": 12,
|
||||
"problem": "one line: what is wrong",
|
||||
"problem": "1–2 short paragraphs: what is wrong and why it fails",
|
||||
"fix": "one line: how to fix it",
|
||||
"suggestion": "exact replacement lines for that location, indented as in the file, or \"\" if no safe replacement",
|
||||
"reference": "https://... or \"\""
|
||||
@@ -157,11 +163,16 @@ containing STRICT JSON, nothing else after it:
|
||||
```
|
||||
|
||||
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.
|
||||
- `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).
|
||||
- `problem` is 1–2 short paragraphs (the inline comment shows it verbatim).
|
||||
Lead with the consequence (security / data loss / perf / etc.), then the cause.
|
||||
- At most ~15 findings, highest severity first.
|
||||
- If the diff is clean, output `{"summary":"...","findings":[]}`.
|
||||
- If the diff is clean, output `{"summary":"...","summary_changes":[],"risks":[],"findings":[]}`.
|
||||
- Do NOT repeat anything in `prior_reviews`.
|
||||
- The JSON block must be the LAST thing in your message — the Python shell parses
|
||||
the last ```json fenced block from your output. If you run out of context/steps
|
||||
|
||||
+300
-77
@@ -156,28 +156,85 @@ def parse_text_blocks(content: list) -> str:
|
||||
return "\n".join(out).strip()
|
||||
|
||||
|
||||
def format_review_body(findings: str, model: str, sha: str, summary: str = "", usage_section: str = "") -> str:
|
||||
def format_review_body(
|
||||
findings: str,
|
||||
model: str,
|
||||
sha: str,
|
||||
summary: str = "",
|
||||
usage_section: str = "",
|
||||
*,
|
||||
summary_changes: list[str] | None = None,
|
||||
risks: list[str] | None = None,
|
||||
findings_for_table: list[dict] | None = None,
|
||||
inline_count: int = 0,
|
||||
) -> str:
|
||||
"""Format the posted review summary body.
|
||||
|
||||
`findings` is the bullet text for findings that could NOT be anchored inline
|
||||
(or, on the legacy/no-inline path, the whole review). Empty -> "No issues
|
||||
found.". `summary` (optional, opencode engine) is rendered as a "Summary"
|
||||
section right under the header. `usage_section` (optional, shown only when
|
||||
the PR carries the `AI-USAGE` label) is rendered between the summary and the
|
||||
findings bullets. The hidden sha marker is always appended for the dedupe
|
||||
pass.
|
||||
Layout (per the operator's format guide):
|
||||
|
||||
* Header line (``🤖 AI Review …``).
|
||||
* **Summary of Changes** — 2–4 bullets of what the PR introduces
|
||||
(`summary_changes`); falls back to the opencode prose `summary` if
|
||||
the agent didn't emit the list.
|
||||
* **Key Risks & Concerns** — bullets of potential bugs/edge cases
|
||||
found across the diff (`risks`).
|
||||
* **Findings Overview** — a Markdown table (severity / location /
|
||||
one-line problem) covering ALL findings, anchored or not.
|
||||
* Unanchored bullets — findings with no post-change line to anchor
|
||||
(the inline ones are posted separately as Gitea review comments).
|
||||
* AI Usage & Run Details — wrapped in a ``<details>`` collapsible so
|
||||
the body stays scannable; cost lines stay inside it.
|
||||
* Hidden SHA marker — for the dedupe pass.
|
||||
|
||||
Empty `summary_changes` + empty `risks` + empty `summary` collapse into
|
||||
a single "Summary of Changes: _no summary provided._" line so the body
|
||||
never looks half-rendered.
|
||||
"""
|
||||
header = REVIEW_HEADER.format(model=model, sha=sha[:8] if sha else "unknown")
|
||||
findings = (findings or "").strip()
|
||||
if not findings:
|
||||
findings = "No issues found."
|
||||
marker = SHA_MARKER.format(sha=sha) if sha else ""
|
||||
parts = [header]
|
||||
if summary:
|
||||
parts.append(summary.strip())
|
||||
parts: list[str] = [header]
|
||||
|
||||
# --- Summary of Changes ---
|
||||
sc = list(summary_changes or [])
|
||||
if not sc and summary:
|
||||
sc = _string_list(summary)
|
||||
if sc:
|
||||
sc = sc[:4]
|
||||
items = "\n".join(f"- {item}" for item in sc)
|
||||
parts.append(f"### Summary of Changes\n\n{items}")
|
||||
else:
|
||||
parts.append("### Summary of Changes\n\n_No summary provided._")
|
||||
|
||||
# --- Key Risks & Concerns ---
|
||||
rs = list(risks or [])
|
||||
if rs:
|
||||
items = "\n".join(f"- {item}" for item in rs)
|
||||
parts.append(f"### Key Risks & Concerns\n\n{items}")
|
||||
else:
|
||||
parts.append("### Key Risks & Concerns\n\n_None identified._")
|
||||
|
||||
# --- Findings Overview (table) ---
|
||||
table = findings_table(findings_for_table or [])
|
||||
if table:
|
||||
n_inline = inline_count
|
||||
n_total = len(findings_for_table or [])
|
||||
if n_inline:
|
||||
heading = f"### Findings Overview\n\n_{n_inline} inline comment(s); {n_total} total._"
|
||||
else:
|
||||
heading = f"### Findings Overview\n\n_{n_total} finding(s)._"
|
||||
parts.append(f"{heading}\n\n{table}")
|
||||
|
||||
# --- Unanchored bullets ---
|
||||
fb = (findings or "").strip()
|
||||
if fb:
|
||||
parts.append(fb)
|
||||
|
||||
# --- Collapsible usage ---
|
||||
if usage_section:
|
||||
parts.append(usage_section.strip())
|
||||
parts.append(findings)
|
||||
|
||||
# --- Hidden marker ---
|
||||
marker = SHA_MARKER.format(sha=sha) if sha else ""
|
||||
|
||||
body = "\n\n".join(parts)
|
||||
if marker:
|
||||
body += f"\n{marker}"
|
||||
@@ -608,38 +665,73 @@ def salvage_summary(text: str, max_chars: int = SALVAGE_MAX_CHARS) -> str:
|
||||
)
|
||||
|
||||
|
||||
def parse_review_output(text: str) -> tuple[str, list[dict]]:
|
||||
"""Parse the opengine's stdout into (summary, findings).
|
||||
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).
|
||||
|
||||
Accepts `{"summary": "...", "findings": [...]}` (the opencode pragent agent),
|
||||
`{"findings": [...]}`, or a bare `[...]` of finding dicts. `summary` defaults
|
||||
to "". 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.
|
||||
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.
|
||||
"""
|
||||
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] = []
|
||||
findings_raw = None
|
||||
if isinstance(data, dict):
|
||||
summary = str(data.get("summary", "") or "").strip()
|
||||
findings = data.get("findings")
|
||||
summary_changes = _string_list(data.get("summary_changes"))
|
||||
risks = _string_list(data.get("risks"))
|
||||
findings_raw = data.get("findings")
|
||||
elif isinstance(data, list):
|
||||
# Bare array: each item is a finding; no summary.
|
||||
summary = ""
|
||||
findings = data
|
||||
# Bare array: each item is a finding; no summary/sections.
|
||||
findings_raw = data
|
||||
else:
|
||||
return "", []
|
||||
return "", [], [], []
|
||||
out = []
|
||||
if isinstance(findings, list):
|
||||
for f in findings:
|
||||
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
|
||||
return summary, out, summary_changes, risks
|
||||
|
||||
|
||||
def _string_list(value) -> list[str]:
|
||||
"""Coerce a JSON value into a list of non-empty strings.
|
||||
|
||||
Accepts a list of strings, a single string (split on lines/bullets), or
|
||||
anything else (returns []). Used for `summary_changes` and `risks`,
|
||||
which some agents emit as one big string instead of a list.
|
||||
"""
|
||||
if isinstance(value, list):
|
||||
return [str(v).strip() for v in value if str(v).strip()]
|
||||
if isinstance(value, str):
|
||||
s = value.strip()
|
||||
if not s:
|
||||
return []
|
||||
# Split on newlines OR on lines that start with "- " / "* " (markdown
|
||||
# bullets). Strip the bullet markers.
|
||||
out: list[str] = []
|
||||
for line in s.splitlines():
|
||||
line = line.strip()
|
||||
if not line:
|
||||
continue
|
||||
if line[:2] in ("- ", "* "):
|
||||
line = line[2:].strip()
|
||||
if line:
|
||||
out.append(line)
|
||||
return out
|
||||
return []
|
||||
|
||||
|
||||
def _parse_json_tolerant(text: str) -> dict | list | None:
|
||||
@@ -853,43 +945,175 @@ def _lang_for_path(path: str) -> str:
|
||||
}.get(ext, "")
|
||||
|
||||
|
||||
_SEVERITY_EMOJI = {
|
||||
"critical": "🔴",
|
||||
"high": "🔴",
|
||||
"medium": "🟡",
|
||||
"low": "🔵",
|
||||
"info": "⚪",
|
||||
"nit": "⚪",
|
||||
}
|
||||
|
||||
|
||||
def _severity_badge(severity: str) -> str:
|
||||
"""Render the severity as emoji + uppercase label (e.g. ``🔴 [HIGH]``)."""
|
||||
sev = (severity or "").lower()
|
||||
emoji = _SEVERITY_EMOJI.get(sev, "⚪")
|
||||
label = sev.upper() if sev in {"critical", "high", "medium", "low"} else "INFO"
|
||||
return f"{emoji} [{label}]"
|
||||
|
||||
|
||||
def _format_reference(ref: str) -> str:
|
||||
"""Render a reference URL as a clean Markdown hyperlink.
|
||||
|
||||
``"https://example.com/x"`` → ``"[example.com/x](https://example.com/x)"``.
|
||||
Accepts the bare URL form so older findings still render readably; drops
|
||||
anything that doesn't look like a URL rather than embedding raw text in
|
||||
parens (the spec says: never print raw URLs).
|
||||
"""
|
||||
ref = (ref or "").strip()
|
||||
if not ref:
|
||||
return ""
|
||||
if not (ref.startswith("http://") or ref.startswith("https://")):
|
||||
# Non-URL text (e.g. a CVE id, a doc title). Render as plain text label.
|
||||
return f"[{ref}]({ref})"
|
||||
# Strip the scheme + www. for the visible label so the link text is short.
|
||||
visible = ref
|
||||
for prefix in ("https://", "http://"):
|
||||
if visible.startswith(prefix):
|
||||
visible = visible[len(prefix):]
|
||||
break
|
||||
if visible.startswith("www."):
|
||||
visible = visible[4:]
|
||||
# Drop trailing slash + truncate any path noise past 60 chars.
|
||||
visible = visible.rstrip("/")
|
||||
if len(visible) > 60:
|
||||
visible = visible[:57] + "…"
|
||||
return f"[{visible}]({ref})"
|
||||
|
||||
|
||||
def inline_comment_body(f: dict) -> str:
|
||||
"""Render one finding as a positional review-comment body.
|
||||
|
||||
Includes a fenced suggested-fix block only if the model produced non-empty
|
||||
replacement code. The fence is tagged with the file's language (via
|
||||
`_lang_for_path`) so Gitea syntax-highlights it — Gitea 1.26.x has no
|
||||
GitHub-style "Apply suggestion" button (```suggestion is just an
|
||||
unknown-language block there → plain monospace), so a language-tagged block
|
||||
is strictly more readable and loses nothing. Appends a `📎 ref:` link when
|
||||
the finding carries a `reference` URL.
|
||||
Shape:
|
||||
* Severity badge with emoji (🔴 HIGH / 🟡 MEDIUM / 🔵 LOW / ⚪ INFO).
|
||||
* 1–2 short paragraphs: ``problem`` + optional ``fix``.
|
||||
* ``suggestion`` block (Gitea/Forgejo apply-on-click) when the model
|
||||
produced replacement code. Language-tagged fences are reserved for
|
||||
cross-file patterns the suggestion block can't carry.
|
||||
* Reference as a Markdown hyperlink (``[label](url)``) — never a raw URL.
|
||||
* No per-comment token attribution: the PR-level collapsible carries
|
||||
all telemetry; inline comments stay focused on the code.
|
||||
"""
|
||||
sev = f["severity"].upper()
|
||||
body = f"**[{sev}]** {f['problem']}"
|
||||
if f["fix"]:
|
||||
body += f"\n\nFix: {f['fix']}"
|
||||
if f["suggestion"]:
|
||||
lang = _lang_for_path(f.get("path", ""))
|
||||
fence = f"```{lang}" if lang else "```"
|
||||
body += f"\n\n{fence}\n{f['suggestion']}\n```"
|
||||
ref = f.get("reference", "")
|
||||
if ref:
|
||||
body += f"\n\n📎 ref: {ref}"
|
||||
tok = f.get("_tok_attrib")
|
||||
if tok is not None:
|
||||
pct = (f.get("_tok_pct", 0.0) or 0.0) * 100
|
||||
body += f"\n\n🪙 ~{tok} tok ({pct:.0f}% · attributed output)"
|
||||
badge = _severity_badge(f.get("severity", "medium"))
|
||||
body = f"{badge} {f.get('problem', '').strip()}"
|
||||
fix = (f.get("fix") or "").strip()
|
||||
if fix:
|
||||
body += f"\n\n**Fix:** {fix}"
|
||||
suggestion = (f.get("suggestion") or "").strip()
|
||||
if suggestion:
|
||||
# `suggestion` fence is the standard one-click-apply block in
|
||||
# Gitea/Forgejo/GitHub. The agent's replacement lines must already be
|
||||
# indented as in the target file.
|
||||
body += f"\n\n```suggestion\n{suggestion}\n```"
|
||||
ref_md = _format_reference(f.get("reference", ""))
|
||||
if ref_md:
|
||||
body += f"\n\n🔗 **Reference:** {ref_md}"
|
||||
return body
|
||||
|
||||
|
||||
def summary_bullets(findings: list[dict]) -> str:
|
||||
"""Render unanchored findings as summary-body bullets (no line anchor)."""
|
||||
"""Render unanchored findings as PR-level bullets.
|
||||
|
||||
Used for findings that couldn't be anchored to a post-change line (no
|
||||
inline comment posted). Each bullet carries severity, location, problem,
|
||||
fix, and a Markdown-linked reference.
|
||||
"""
|
||||
lines = []
|
||||
for f in findings:
|
||||
loc = f"{f['path']}:{f['line']}" if f["line"] else f["path"]
|
||||
fix = f" — fix: {f['fix']}" if f["fix"] else ""
|
||||
ref = f" ({f.get('reference', '')})" if f.get("reference") else ""
|
||||
lines.append(f"- **[{f['severity'].upper()}]** `{loc}` — {f['problem']}{fix}{ref}")
|
||||
badge = _severity_badge(f.get("severity", "medium"))
|
||||
problem = f.get("problem", "").strip()
|
||||
body = f"- {badge} `{loc}` — {problem}"
|
||||
fix = (f.get("fix") or "").strip()
|
||||
if fix:
|
||||
body += f"\n - **Fix:** {fix}"
|
||||
ref_md = _format_reference(f.get("reference", ""))
|
||||
if ref_md:
|
||||
body += f"\n - 🔗 **Reference:** {ref_md}"
|
||||
lines.append(body)
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
def findings_table(findings: list[dict]) -> str:
|
||||
"""Render ALL findings as a Markdown table for the PR-level comment.
|
||||
|
||||
Columns: severity emoji, location (path:line), and a one-line summary.
|
||||
Findings with empty location collapse to just the severity + summary.
|
||||
"""
|
||||
if not findings:
|
||||
return ""
|
||||
header = "| Severity | Location | Finding |\n|---|---|---|"
|
||||
rows = []
|
||||
for f in findings:
|
||||
badge = _severity_badge(f.get("severity", "medium"))
|
||||
path = (f.get("path") or "").strip()
|
||||
line = f.get("line")
|
||||
loc = f"`{path}:{line}`" if line else (f"`{path}`" if path else "_(no location)_")
|
||||
problem = (f.get("problem") or "").strip()
|
||||
# Escape pipes inside the finding text so the table stays valid.
|
||||
problem_esc = problem.replace("|", "\\|").replace("\n", " ")
|
||||
rows.append(f"| {badge} | {loc} | {problem_esc} |")
|
||||
return "\n".join([header, *rows])
|
||||
|
||||
|
||||
def _render_collapsible_usage(usage: dict | None, model: str, config: dict | None) -> str:
|
||||
"""Render the telemetry as a collapsible ``<details>`` block.
|
||||
|
||||
Empty string when `usage` is None. The cost-equivalent line is always
|
||||
shown (it's the operator's budgeting signal). The `actual` line is shown
|
||||
but the FREE-TIER note is collapsed into a single short clause.
|
||||
"""
|
||||
if not usage:
|
||||
return ""
|
||||
dur = usage.get("duration_s")
|
||||
dur_s = f"{dur}s" if dur is not None else "?"
|
||||
actual = usage.get("cost") or 0.0
|
||||
actual_s = f"${actual:.4f}" if actual else "$0.00"
|
||||
actual_note = " (headroom glm-5.2:cloud — free tier)" if not actual else ""
|
||||
price_key, price_err = _resolve_price_target(config)
|
||||
from cost_model import PRICES
|
||||
eq = equivalent_cost(usage, price_key)
|
||||
eq_s = f"${eq:.4f}" if eq else "$0.00"
|
||||
eq_label = PRICES[price_key].name
|
||||
eq_note = (
|
||||
f" _(price target: `{price_key}`; {price_err})_"
|
||||
if price_err else ""
|
||||
)
|
||||
in_tok = usage.get("input", 0)
|
||||
out_tok = usage.get("output", 0)
|
||||
reason_tok = usage.get("reasoning", 0)
|
||||
cache_r = usage.get("cache_read", 0)
|
||||
cache_w = usage.get("cache_write", 0)
|
||||
total = usage.get("total", 0)
|
||||
scope = (
|
||||
"Whole-repo checkout at head sha (agent can read any file + run "
|
||||
"linters, not just the diff) — input tokens include files read "
|
||||
"beyond the diff. Per-comment output is *attributed* (one model pass "
|
||||
"produces all findings; output split by each finding's body weight)."
|
||||
)
|
||||
lines = [
|
||||
"<details>",
|
||||
"<summary>🔋 AI Usage & Run Details</summary>",
|
||||
"",
|
||||
f"- **Model / Engine**: `{model}` · opencode · {usage.get('steps', 0)} steps · {dur_s}",
|
||||
f"- **Total Tokens**: {in_tok} in / {out_tok} out ({reason_tok} reasoning, cache {cache_r} read / {cache_w} write, {total} total)",
|
||||
f"- **Est. cost on {eq_label}**: {eq_s}{eq_note}",
|
||||
f"- **Actual**: {actual_s}{actual_note}",
|
||||
f"- **Scope**: {scope}",
|
||||
"",
|
||||
"</details>",
|
||||
]
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
@@ -1442,7 +1666,7 @@ def review_pr(
|
||||
prior_reviews=prior, model=oc_model,
|
||||
compression_note=compression_note,
|
||||
)
|
||||
review_summary, findings = parse_review_output(stdout)
|
||||
review_summary, findings, summary_changes, risks = 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
|
||||
@@ -1454,9 +1678,7 @@ def review_pr(
|
||||
file=sys.stderr, flush=True,
|
||||
)
|
||||
salvaged = salvage_summary(stdout)
|
||||
usage_section = ""
|
||||
if report_usage and usage:
|
||||
usage_section = format_usage_section(usage, [], model, config=config)
|
||||
usage_section = _render_collapsible_usage(usage, model, config=config) if report_usage else ""
|
||||
post_review(api, repo, index, token, format_review_body(
|
||||
salvaged or "AI review produced no parseable output.",
|
||||
model, sha, usage_section=usage_section))
|
||||
@@ -1491,31 +1713,32 @@ def review_pr(
|
||||
flush=True,
|
||||
)
|
||||
|
||||
# Attribute output tokens to each finding (mutates finding dicts) so
|
||||
# inline comments + the usage table can show a per-comment estimate.
|
||||
# Only meaningful when we have measured usage AND the PR asked for it.
|
||||
usage_section = ""
|
||||
# Compute attribution so inline comments + the table can show per-comment
|
||||
# estimates. Only meaningful when we have measured usage AND the PR asked
|
||||
# for it.
|
||||
if report_usage and usage and usage.get("output"):
|
||||
compute_attribution(findings, usage["output"])
|
||||
usage_section = format_usage_section(usage, findings, model, config=config)
|
||||
usage_section = _render_collapsible_usage(usage, model, config=config) if report_usage else ""
|
||||
|
||||
anchors = parse_diff_anchors(diff)
|
||||
anchored, unanchored = split_findings(findings, anchors)
|
||||
|
||||
# Summary body: the unanchored bullets (or "No issues found."), plus a
|
||||
# one-line note when inline comments were posted so the summary isn't
|
||||
# empty-looking. The opencode engine also carries a prose summary.
|
||||
# Summary body: unanchored bullets fall through to a "Unanchored notes"
|
||||
# section; the structured Findings Overview table covers both anchored
|
||||
# + unanchored so reviewers see the full set even if inline comments
|
||||
# are collapsed.
|
||||
bullets = summary_bullets(unanchored)
|
||||
summary_parts = []
|
||||
if anchored:
|
||||
summary_parts.append(f"_{len(anchored)} inline comment(s) posted below._")
|
||||
if bullets:
|
||||
summary_parts.append(bullets)
|
||||
if not summary_parts:
|
||||
summary_parts.append("No issues found.")
|
||||
summary_parts.append("### Unanchored Notes\n\n" + bullets)
|
||||
summary_body = format_review_body(
|
||||
"\n\n".join(summary_parts), model, sha,
|
||||
summary=review_summary, usage_section=usage_section,
|
||||
summary=review_summary,
|
||||
usage_section=usage_section,
|
||||
summary_changes=summary_changes,
|
||||
risks=risks,
|
||||
findings_for_table=findings,
|
||||
inline_count=len(anchored),
|
||||
)
|
||||
|
||||
post_inline_review(api, repo, index, token, summary_body, anchored)
|
||||
|
||||
+160
-30
@@ -14,8 +14,10 @@ from ai_review import ( # noqa: E402
|
||||
_balanced_json_substring,
|
||||
_extract_first_json_object,
|
||||
_last_balanced_json,
|
||||
_render_collapsible_usage,
|
||||
build_user_prompt,
|
||||
compute_attribution,
|
||||
findings_table,
|
||||
format_review_body,
|
||||
format_usage_section,
|
||||
inline_comment_body,
|
||||
@@ -108,17 +110,25 @@ def test_format_review_body_findings():
|
||||
assert "pragent pilot" in body
|
||||
assert "glm-5.2:cloud" in body
|
||||
assert "`abcdef12`" in body # 8-char sha
|
||||
assert "- [high] x:1" in body
|
||||
# New layout: always emits Summary of Changes + Key Risks. Findings table
|
||||
# only shows when findings_for_table is passed (callers pass the actual
|
||||
# list of finding dicts; plain-string findings arg renders as bullets).
|
||||
assert "### Summary of Changes" in body
|
||||
assert "### Key Risks & Concerns" in body
|
||||
|
||||
|
||||
def test_format_review_body_empty_findings():
|
||||
body = format_review_body("", "glm-5.2:cloud", "abcdef1234567890")
|
||||
assert "No issues found." in body
|
||||
# No summary → "no summary provided" sentinel; Findings table absent
|
||||
# because no findings were passed.
|
||||
assert "_No summary provided._" in body
|
||||
assert "_None identified._" in body
|
||||
assert "### Findings Overview" not in body
|
||||
|
||||
|
||||
def test_format_review_body_whitespace_findings():
|
||||
body = format_review_body(" \n ", "glm-5.2:cloud", "abcdef1234567890")
|
||||
assert "No issues found." in body
|
||||
assert "_No summary provided._" in body
|
||||
|
||||
|
||||
def test_format_review_body_no_sha():
|
||||
@@ -274,33 +284,98 @@ def test_split_findings_by_anchor():
|
||||
def test_inline_comment_body_with_suggestion():
|
||||
f = {"severity": "high", "path": "a", "line": 1, "problem": "bad", "fix": "swap", "suggestion": "good()"}
|
||||
body = inline_comment_body(f)
|
||||
assert "**[HIGH]**" in body
|
||||
# Severity emoji + bracketed label.
|
||||
assert "🔴 [HIGH]" in body
|
||||
assert "bad" in body
|
||||
# no extension → bare fence (Gitea 1.26.x has no apply-suggestion; we tag
|
||||
# with the file language for highlighting instead of ```suggestion)
|
||||
assert "```\ngood()\n```" in body
|
||||
# Standard ```suggestion fence (Gitea/Forgejo apply-on-click).
|
||||
assert "```suggestion\ngood()\n```" in body
|
||||
assert "good()" in body
|
||||
|
||||
|
||||
def test_inline_comment_body_suggestion_lang_tagged():
|
||||
def test_inline_comment_body_suggestion_not_lang_tagged():
|
||||
# Per the format spec, the suggestion fence is ALWAYS ```suggestion —
|
||||
# never a language-tagged fence (those are reserved for cross-file
|
||||
# pattern illustrations, which we don't emit here).
|
||||
f = {"severity": "high", "path": "src/Foo.java", "line": 1,
|
||||
"problem": "bad", "fix": "swap", "suggestion": "good();"}
|
||||
body = inline_comment_body(f)
|
||||
assert "```java\ngood();\n```" in body
|
||||
assert "```suggestion\ngood();\n```" in body
|
||||
assert "```java" not in body
|
||||
|
||||
|
||||
def test_inline_comment_body_no_suggestion():
|
||||
f = {"severity": "low", "path": "a", "line": 1, "problem": "p", "fix": "f", "suggestion": ""}
|
||||
body = inline_comment_body(f)
|
||||
assert "```" not in body
|
||||
assert "Fix: f" in body
|
||||
assert "**Fix:** f" in body
|
||||
|
||||
|
||||
def test_inline_comment_body_severity_emoji_mapping():
|
||||
cases = [
|
||||
("critical", "🔴 [CRITICAL]"),
|
||||
("high", "🔴 [HIGH]"),
|
||||
("medium", "🟡 [MEDIUM]"),
|
||||
("low", "🔵 [LOW]"),
|
||||
("info", "⚪ [INFO]"),
|
||||
("nit", "⚪ [INFO]"), # "nit" maps to the INFO label
|
||||
("bogus", "⚪ [INFO]"), # unknown severity falls back to INFO
|
||||
]
|
||||
for sev, badge in cases:
|
||||
f = {"severity": sev, "path": "a", "line": 1, "problem": "p", "fix": "",
|
||||
"suggestion": "", "reference": ""}
|
||||
assert badge in inline_comment_body(f), f"{sev} → {badge}"
|
||||
|
||||
|
||||
def test_inline_comment_body_no_token_attribution():
|
||||
# Per spec: no per-comment 🪙 token attribution line.
|
||||
f = {"severity": "high", "path": "a", "line": 1, "problem": "p",
|
||||
"fix": "f", "suggestion": "", "reference": "",
|
||||
"_tok_attrib": 1234, "_tok_pct": 0.3}
|
||||
body = inline_comment_body(f)
|
||||
assert "🪙" not in body
|
||||
assert "tok" not in body.lower().split("fix")[0] # only in fix is OK
|
||||
assert "attributed" not in body
|
||||
|
||||
|
||||
def test_summary_bullets_format():
|
||||
fs = [{"severity": "high", "path": "a.py", "line": 7, "problem": "p", "fix": "f", "suggestion": ""}]
|
||||
b = summary_bullets(fs)
|
||||
assert "- **[HIGH]**" in b
|
||||
assert "🔴 [HIGH]" in b
|
||||
assert "`a.py:7`" in b
|
||||
assert "**Fix:** f" in b
|
||||
|
||||
|
||||
def test_summary_bullets_with_reference_link():
|
||||
fs = [{"severity": "medium", "path": "x", "line": 1, "problem": "p",
|
||||
"fix": "", "suggestion": "", "reference": "https://owasp.org/x"}]
|
||||
b = summary_bullets(fs)
|
||||
assert "🔗 **Reference:** [owasp.org/x](https://owasp.org/x)" in b
|
||||
assert "https://owasp.org/x" in b # URL preserved
|
||||
|
||||
|
||||
def test_findings_table_renders_table():
|
||||
fs = [
|
||||
{"severity": "high", "path": "a.py", "line": 1, "problem": "bug", "fix": "", "suggestion": "", "reference": ""},
|
||||
{"severity": "low", "path": "b.go", "line": 9, "problem": "nit", "fix": "", "suggestion": "", "reference": ""},
|
||||
]
|
||||
t = findings_table(fs)
|
||||
assert t.startswith("| Severity | Location | Finding |")
|
||||
assert "|---|---|---|" in t
|
||||
assert "🔴 [HIGH]" in t
|
||||
assert "🔵 [LOW]" in t
|
||||
assert "`a.py:1`" in t
|
||||
assert "`b.go:9`" in t
|
||||
|
||||
|
||||
def test_findings_table_escapes_pipes():
|
||||
fs = [{"severity": "high", "path": "a", "line": 1,
|
||||
"problem": "uses | inside", "fix": "", "suggestion": "", "reference": ""}]
|
||||
t = findings_table(fs)
|
||||
assert "uses \\| inside" in t
|
||||
|
||||
|
||||
def test_findings_table_empty():
|
||||
assert findings_table([]) == ""
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -376,7 +451,7 @@ def test_parse_review_output_summary_and_findings():
|
||||
"]}",
|
||||
"\n```",
|
||||
)
|
||||
summary, fs = parse_review_output("".join(txt))
|
||||
summary, fs, *_ = parse_review_output("".join(txt))
|
||||
assert "eval()" in summary
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["severity"] == "critical"
|
||||
@@ -386,16 +461,16 @@ def test_parse_review_output_summary_and_findings():
|
||||
|
||||
def test_parse_review_output_bare_findings_no_summary():
|
||||
txt = '```json\n{"findings":[{"severity":"low","path":"a","line":1,"problem":"p"}]}\n```'
|
||||
summary, fs = parse_review_output(txt)
|
||||
summary, fs, *_ = parse_review_output(txt)
|
||||
assert summary == ""
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["reference"] == "" # default
|
||||
|
||||
|
||||
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():
|
||||
@@ -405,7 +480,7 @@ def test_parse_review_output_uses_last_json_block():
|
||||
"more prose\n"
|
||||
"```json\n{\"summary\":\"real\",\"findings\":[{\"path\":\"y\",\"line\":2,\"severity\":\"high\"}]}\n```"
|
||||
)
|
||||
summary, fs = parse_review_output(txt)
|
||||
summary, fs, *_ = parse_review_output(txt)
|
||||
assert summary == "real"
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["path"] == "y"
|
||||
@@ -463,7 +538,7 @@ def test_parse_review_output_unfenced_at_tail():
|
||||
'{"severity":"high","path":"VoidProtection.java","line":162,'
|
||||
'"problem":"drop duplication race","fix":"use ItemMeta","suggestion":"","reference":""}]}'
|
||||
)
|
||||
summary, fs = parse_review_output(txt)
|
||||
summary, fs, *_ = parse_review_output(txt)
|
||||
assert "void protection" in summary.lower()
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["path"] == "VoidProtection.java"
|
||||
@@ -474,7 +549,7 @@ def test_parse_review_output_bare_array_at_tail():
|
||||
"All wrapped up.\n"
|
||||
'[{"severity":"low","path":"a","line":1,"problem":"p","fix":"","suggestion":"","reference":""}]'
|
||||
)
|
||||
summary, fs = parse_review_output(txt)
|
||||
summary, fs, *_ = parse_review_output(txt)
|
||||
assert summary == ""
|
||||
assert len(fs) == 1
|
||||
|
||||
@@ -522,13 +597,26 @@ def test_inline_comment_body_renders_reference():
|
||||
f = {"severity": "high", "path": "a", "line": 1, "problem": "p", "fix": "f",
|
||||
"suggestion": "", "reference": "https://cve.example/X"}
|
||||
body = inline_comment_body(f)
|
||||
assert "📎 ref: https://cve.example/X" in body
|
||||
# Per spec: Markdown hyperlink, not raw URL.
|
||||
assert "🔗 **Reference:** [cve.example/X](https://cve.example/X)" in body
|
||||
|
||||
|
||||
def test_inline_comment_body_no_reference_no_ref_line():
|
||||
f = {"severity": "low", "path": "a", "line": 1, "problem": "p", "fix": "",
|
||||
"suggestion": "", "reference": ""}
|
||||
assert "📎 ref" not in inline_comment_body(f)
|
||||
assert "🔗" not in inline_comment_body(f)
|
||||
assert "Reference:" not in inline_comment_body(f)
|
||||
|
||||
|
||||
def test_inline_comment_body_reference_truncates_long_url():
|
||||
f = {"severity": "high", "path": "a", "line": 1, "problem": "p", "fix": "",
|
||||
"suggestion": "",
|
||||
"reference": "https://very-long-domain.example.com/some/very/long/path/that/exceeds/the/sixty/char/limit/x"}
|
||||
body = inline_comment_body(f)
|
||||
# Visible label is truncated to ≤60 chars (ellipsis added).
|
||||
assert "…" in body
|
||||
# But the underlying URL is preserved verbatim inside the link target.
|
||||
assert "very-long-domain.example.com" in body
|
||||
|
||||
|
||||
def test_summary_bullets_renders_reference():
|
||||
@@ -585,13 +673,15 @@ def test_compute_attribution_noop_on_empty_or_zero_budget():
|
||||
assert "_tok_attrib" not in fs[0]
|
||||
|
||||
|
||||
def test_inline_comment_body_with_attribution_line():
|
||||
def test_inline_comment_body_no_attribution_line():
|
||||
# Per spec: NO per-comment token attribution — that telemetry lives in the
|
||||
# collapsible block on the PR-level comment.
|
||||
f = {"severity": "high", "path": "a", "line": 1, "problem": "bad", "fix": "swap",
|
||||
"suggestion": "", "_tok_attrib": 180, "_tok_pct": 0.29}
|
||||
body = inline_comment_body(f)
|
||||
assert "🪙 ~180 tok" in body
|
||||
assert "29%" in body
|
||||
assert "attributed output" in body
|
||||
assert "🪙" not in body
|
||||
assert "tok" not in body
|
||||
assert "attributed" not in body
|
||||
|
||||
|
||||
def test_inline_comment_body_no_attribution_no_coin_line():
|
||||
@@ -643,14 +733,14 @@ def test_format_usage_section_cost_nonzero():
|
||||
assert "billed by provider" in sec
|
||||
|
||||
|
||||
def test_format_review_body_usage_section_between_summary_and_findings():
|
||||
def test_format_review_body_usage_section_below_findings():
|
||||
# New layout: header → Summary of Changes → Key Risks → findings → usage.
|
||||
usage_sec = "## 🔋 AI usage\n\n- model: `m`"
|
||||
body = format_review_body("- [high] x:1 — b", "glm-5.2:cloud", "abcdef1234567890",
|
||||
summary="This PR is risky.", usage_section=usage_sec)
|
||||
# order: header < summary < usage < findings < marker
|
||||
assert body.index("risky.") < body.index("AI usage")
|
||||
assert body.index("AI usage") < body.index("[high]")
|
||||
assert body.index("[high]") < body.index("<!-- pragent:sha=")
|
||||
assert body.index("risky.") < body.index("[high]")
|
||||
assert body.index("[high]") < body.index("AI usage")
|
||||
assert body.index("AI usage") < body.index("<!-- pragent:sha=")
|
||||
assert "## 🔋 AI usage" in body
|
||||
|
||||
|
||||
@@ -658,6 +748,46 @@ def test_format_review_body_no_usage_section_omitted():
|
||||
body = format_review_body("- [high] x:1 — b", "glm-5.2:cloud", "abcdef1234567890")
|
||||
assert "AI usage" not in body
|
||||
|
||||
|
||||
def test_format_review_body_with_summary_changes_and_risks():
|
||||
body = format_review_body(
|
||||
"", "glm-5.2:cloud", "abcdef1234567890",
|
||||
summary_changes=["Adds void-death item rescue.", "Adds chunk-loader pause/rename."],
|
||||
risks=["Mob spawn leak in force-loaded chunks.", "Race on /chunkloader tempo -1."],
|
||||
findings_for_table=[
|
||||
{"severity": "high", "path": "a.py", "line": 1, "problem": "race", "fix": "",
|
||||
"suggestion": "", "reference": ""},
|
||||
],
|
||||
inline_count=1,
|
||||
)
|
||||
assert "### Summary of Changes" in body
|
||||
assert "Adds void-death item rescue." in body
|
||||
assert "Adds chunk-loader pause/rename." in body
|
||||
assert "### Key Risks & Concerns" in body
|
||||
assert "Mob spawn leak in force-loaded chunks." in body
|
||||
assert "### Findings Overview" in body
|
||||
assert "1 inline comment(s); 1 total." in body
|
||||
assert "🔴 [HIGH]" in body
|
||||
assert "`a.py:1`" in body
|
||||
|
||||
|
||||
def test_render_collapsible_usage_contains_details():
|
||||
usage = {
|
||||
"model": "glm-5.2:cloud", "input": 1000, "output": 200, "reasoning": 0,
|
||||
"cache_read": 0, "cache_write": 0, "total": 1200, "steps": 5, "duration_s": 12.0,
|
||||
"cost": 0.0,
|
||||
}
|
||||
block = _render_collapsible_usage(usage, "glm-5.2:cloud", config=None)
|
||||
assert "<details>" in block
|
||||
assert "<summary>🔋 AI Usage & Run Details</summary>" in block
|
||||
assert "</details>" in block
|
||||
assert "glm-5.2:cloud" in block
|
||||
assert "1000 in / 200 out" in block
|
||||
|
||||
|
||||
def test_render_collapsible_usage_empty_when_no_usage():
|
||||
assert _render_collapsible_usage(None, "glm-5.2:cloud", config=None) == ""
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# parse_diff_anchors — empty context lines
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user