From 770581bf53ad13b43d65aee6e6e85cdc617b3157 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 16:12:33 +0000 Subject: [PATCH 1/8] feat(input): add diff_compress module + prior-review compaction helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pure stdlib helpers that shrink what lands in the model prompt: * compress_diff(diff, *, context=2) — re-renders a unified diff so each hunk keeps only unchanged lines on either side of its +/- lines. File headers + hunk headers + +/- lines preserved verbatim. Pure-context hunks dropped (rare but legal — git emits them on whitespace-only diffs). Collapsed gaps of >=5 lines emit a single '@@ … N context line(s) omitted … @@' marker so the reviewer knows code was elided. Smaller gaps stay silent — the marker would be longer than the elision. * extract_finding_bullets(review_body) — pulls the lines of a prior review that look like a pragent finding (- **[SEVERITY]** path:line — …) and drops everything else. The model already has the diff; repeating the prose is just token burn. No I/O, no network. Tolerant of malformed input — never raises. 14 unit tests cover both helpers, including an anchor-preservation check against parse_diff_anchors to guarantee compress-then-anchor still works. Wiring in ai_review/opencode_review lives in the next commit. --- pilot/diff_compress.py | 169 ++++++++++++++++++++ tests/pilot/test_diff_compress.py | 248 ++++++++++++++++++++++++++++++ 2 files changed, 417 insertions(+) create mode 100644 pilot/diff_compress.py create mode 100644 tests/pilot/test_diff_compress.py diff --git a/pilot/diff_compress.py b/pilot/diff_compress.py new file mode 100644 index 0000000..52649e0 --- /dev/null +++ b/pilot/diff_compress.py @@ -0,0 +1,169 @@ +#!/usr/bin/env python3 +r"""pragent pilot — diff compression + prior-review compaction. + +Two pure helpers that shrink what lands in the model prompt without losing +signal: + + * ``compress_diff(diff, *, context=2)`` — re-renders a unified diff so each + hunk keeps only ``context`` unchanged lines on either side of its +/- lines. + The default 2 matches what most reviewers see on GitHub/Gitea, and is + enough to anchor every ``+``/``-`` line and give the reviewer the enclosing + statement. Wider context = more reading; narrower = less. Set + ``context=0`` for +/- only, ``context=-1`` to disable entirely. + + * ``extract_finding_bullets(review_body)`` — pulls the lines of a prior + review that look like a pragent finding (``- **[SEVERITY]** `path:line` — …``) + and drops everything else. The model already has the diff — repeating the + prose ("this PR adds eval() — risky") is just token burn. Bullet-only + priors cut ~75% off prior-review bytes on a typical 4-finding review. + +Stdlib only. No I/O. Tolerant of malformed input — never raises. +""" + +from __future__ import annotations + +import re + +# Diff line types. Order matters: `+++`/ `---` headers and `@@` hunk headers +# are caught before the per-line prefix check. +_FILE_HEADER = re.compile(r"^(diff --git|Index:|---|\+\+\+|@@)") + +# Captures `- [,]` AND `+ [,]` from `@@ -a,b +c,d @@`. We use the +# `+` side to reset the new-line counter; old-side is ignored. +_HUNK_RE = re.compile(r"^@@\s+-\d+(?:,\d+)?\s+\+(\d+)(?:,\d+)?\s+@@") + +# Match a pragent summary-bullet line: `- **[SEVERITY]** \`path:line\` — …`. +# Severity is uppercased critical|high|medium|low per the findings schema. +# We also accept the lower-case form (`- [high]`) used by summary_bullets. +_FINDING_BULLET_RE = re.compile( + r"^\s*-\s*\*?\*?\[(?Pcritical|high|medium|low|CRITICAL|HIGH|MEDIUM|LOW)\]" + r"\*?\*?\s+(?P.+)$" +) + + +def compress_diff(diff: str, *, context: int = 2) -> tuple[str, int, int]: + """Re-render `diff` keeping at most `context` unchanged lines around +/-. + + Args: + diff: unified-diff text (what `gitea .../pulls/{n}.diff` returns). + context: max unchanged lines to keep on each side of a hunk. Use 0 + for +/- only, -1 to disable compression (raw passthrough). + + Returns: + `(text, original_chars, kept_chars)`. `original_chars` is the byte + length of `diff` as given; `kept_chars` is the byte length of `text`. + On parse failure the original is returned unchanged so the worst case + is no improvement, never corruption. + """ + if not diff: + return diff or "", len(diff or ""), len(diff or "") + if context < 0: + return diff, len(diff), len(diff) + + orig = len(diff) + lines = diff.splitlines() + out: list[str] = [] + + # State for the per-file walk. + i = 0 + n = len(lines) + while i < n: + # Copy file headers verbatim until we hit the first `@@` hunk header. + hunk_start = i + while hunk_start < n and not lines[hunk_start].startswith("@@"): + out.append(lines[hunk_start]) + hunk_start += 1 + i = hunk_start + + # Walk hunks, copying headers verbatim and trimming the inside. + while i < n and lines[i].startswith("@@"): + hunk_header = lines[i] + i += 1 + + # Collect the hunk body: every line until the next `@@` / file + # header / EOF. Within the body, classify each line. + body_start = i + while i < n and not _FILE_HEADER.match(lines[i]): + i += 1 + body = lines[body_start:i] + + # Render the body, collapsing long runs of context lines to a + # `@@ … @@` marker so the reviewer still sees that there IS more + # code there, just not in this window. + rendered, _ = _render_hunk_body(body, context=context) + if rendered: + out.append(hunk_header) + out.extend(rendered) + + text = "\n".join(out) + ("\n" if diff.endswith("\n") else "") + if not text: + # splitlines() dropped nothing-but-newlines; fall back to original. + return diff, orig, orig + return text, orig, len(text) + + +def _render_hunk_body(body: list[str], *, context: int) -> tuple[list[str], int]: + """Trim `body` to `context` unchanged lines around the +/- lines. + + Body lines are classified: + - `+` line → keep + - `-` line → keep (paired with a `+` on the new side when both exist) + - `` `` line (or empty context) → keep only within ``context`` of a +/- line + - ``\ No newline at end of file`` → drop (no signal for the reviewer) + + Collapsed gaps of ≥ 5 lines get a single ``@@ … N context line(s) omitted … @@`` + marker so the reviewer knows code was elided. Smaller gaps (1–4 lines) + stay silent — the marker would be longer than the elision. + """ + if context == 0: + # Keep only +/- lines; drop all context. + out = [ln for ln in body if ln.startswith("+") or ln.startswith("-")] + return out, 0 + + # Find the index of every +/- line; a context line is kept if its + # distance to the nearest +/- line is ≤ context. + plus_minus_idx = [ + j for j, ln in enumerate(body) + if ln.startswith("+") or ln.startswith("-") + ] + if not plus_minus_idx: + # No +/- at all (rare — pure-context hunk): drop entirely. + return [], 0 + + keep = set() + for k in plus_minus_idx: + lo = max(0, k - context) + hi = min(len(body) - 1, k + context) + for j in range(lo, hi + 1): + keep.add(j) + + out: list[str] = [] + last_kept = -2 # sentinel: a gap of ≥ 5 between consecutive kept lines triggers a marker + for j, ln in enumerate(body): + if ln.startswith("\\ No newline"): + continue + if j in keep: + if j - last_kept > 5 and last_kept >= 0: + out.append(f"@@ … {j - last_kept - 1} context line(s) omitted … @@") + out.append(ln) + last_kept = j + + return out, len(out) + + +def extract_finding_bullets(review_body: str) -> list[str]: + """Pull the finding-bullet lines out of a prior review body. + + Returns the matching lines verbatim (with their original indentation + + any continuation text), preserving the ``**[SEV]** `path:line` — problem + …`` shape the model emitted. Lines that look like bullets but lack the + severity tag are dropped — the reviewer synthesizes from the matched ones. + """ + if not review_body: + return [] + out = [] + for line in review_body.splitlines(): + m = _FINDING_BULLET_RE.match(line) + if m: + out.append(line.strip()) + return out \ No newline at end of file diff --git a/tests/pilot/test_diff_compress.py b/tests/pilot/test_diff_compress.py new file mode 100644 index 0000000..7af959a --- /dev/null +++ b/tests/pilot/test_diff_compress.py @@ -0,0 +1,248 @@ +"""Unit tests for pragent pilot diff_compress. No network.""" +import os +import sys + +HERE = os.path.dirname(os.path.abspath(__file__)) +ROOT = os.path.abspath(os.path.join(HERE, "..", "..")) +sys.path.insert(0, os.path.join(ROOT, "pilot")) + +import diff_compress # noqa: E402 +from diff_compress import compress_diff, extract_finding_bullets # noqa: E402 + + +# --------------------------------------------------------------------------- +# compress_diff +# --------------------------------------------------------------------------- + + +_DIFF = """\ +diff --git a/src/a.py b/src/a.py +index 1..2 100644 +--- a/src/a.py ++++ b/src/a.py +@@ -1,10 +1,11 @@ + ctx1 +-removed ++added + ctx2 + ctx3 + ctx4 + ctx5 + ctx6 + ctx7 ++extra + ctx8 +@@ -20,3 +21,4 @@ + tail1 + tail2 ++tail3 + tail4 +diff --git a/binary.bin b/binary.bin +new file mode 100644 +index 0..1 +Binary files differ +""" + + +def test_compress_diff_default_context_two(): + text, orig, kept = compress_diff(_DIFF, context=2) + # +/- lines preserved + assert "+added" in text + assert "-removed" in text + assert "+extra" in text + assert "+tail3" in text + # 2 context lines around +/- kept, the rest collapsed + assert "ctx2" in text and "ctx3" in text + assert "ctx4" not in text # outside the +/- window + # Binary files pass through + assert "Binary files differ" in text + # File headers preserved + assert "diff --git a/src/a.py b/src/a.py" in text + assert orig > kept + + +def test_compress_diff_context_zero_strips_context(): + text, orig, kept = compress_diff(_DIFF, context=0) + assert "+added" in text and "-removed" in text and "+extra" in text + # Context lines dropped (only +/- survive) + assert " ctx1" not in text + assert "ctx2" not in text + assert orig > kept + + +def test_compress_diff_negative_disables_compression(): + text, orig, kept = compress_diff(_DIFF, context=-1) + assert text == _DIFF + assert orig == kept + + +def test_compress_diff_collapsed_gap_marker(): + # Two +/- lines separated by 14 context lines, context=2 — the gap between + # them is 10 dropped lines (between the +/- windows), which exceeds the + # 5-line marker threshold. The marker tells the reviewer there's more code + # between the kept hunks. + middle = "\n".join(f" m{i}" for i in range(14)) + "\n" # trailing \n! + diff = ( + "diff --git a/x.py b/x.py\n" + "--- a/x.py\n" + "+++ b/x.py\n" + "@@ -1,21 +1,23 @@\n" + + " c1\n c2\n" # ctx near +a (kept with context=2) + + "+a\n" + + middle + + "+b\n" + + " c1\n c2\n" # ctx near +b (kept with context=2) + ) + text, _, _ = compress_diff(diff, context=2) + assert "+a" in text and "+b" in text + assert "@@ …" in text and "context line(s) omitted" in text + + +def test_compress_diff_strips_no_newline_marker(): + diff = ( + "diff --git a/x.py b/x.py\n" + "--- a/x.py\n" + "+++ b/x.py\n" + "@@ -1,2 +1,2 @@\n" + " a\n" + "-b\n" + "\\ No newline at end of file\n" + "+c\n" + "\\ No newline at end of file\n" + ) + text, _, _ = compress_diff(diff, context=2) + assert "\\ No newline" not in text + assert "-b" in text and "+c" in text + + +def test_compress_diff_empty_and_none(): + text, orig, kept = compress_diff("", context=2) + assert text == "" + assert orig == 0 and kept == 0 + text, orig, kept = compress_diff(None, context=2) # type: ignore[arg-context] + assert text == "" + assert orig == 0 and kept == 0 + + +def test_compress_diff_pure_context_hunk_drops_body(): + # A hunk that's *only* context lines (rare but legal — `git diff` emits + # these when the post-image differs only in whitespace outside the visible + # hunk) collapses entirely: file headers stay, the empty hunk header + # itself drops. The reviewer doesn't need to re-read unchanged code. + diff = ( + "diff --git a/x.py b/x.py\n" + "--- a/x.py\n" + "+++ b/x.py\n" + "@@ -1,3 +1,3 @@\n" + " a\n" + " b\n" + " c\n" + ) + text, _, _ = compress_diff(diff, context=2) + assert text == "diff --git a/x.py b/x.py\n--- a/x.py\n+++ b/x.py\n" + assert "@@ -1,3" not in text # empty hunk header dropped + + +def test_compress_diff_wide_window_keeps_more_context(): + narrow, _, _ = compress_diff(_DIFF, context=0) + wide, _, wide_kept = compress_diff(_DIFF, context=10) + assert wide_kept > len(narrow) + + +# --------------------------------------------------------------------------- +# extract_finding_bullets +# --------------------------------------------------------------------------- + + +_BODY = """\ +🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `abcdef12` + +Adds the salvavoid void-death item-rescue module. Risk is moderate on the +PlayerDeathEvent item/inventory path. New findings (not in prior review): +orphaned chest left in world on rescue failure, missing module-enabled check. + +- **[HIGH]** `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:162` — drop duplication race. fix: use ItemMeta to write inventory once. (ref: https://example.com) +- **[MEDIUM]** `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:67` — O(n^2) spiral. fix: cap radius. (https://example.com/spiral) +- **[LOW]** `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:3` — package-info javadoc missing. + +_4 inline comment(s) posted below._ + +""" + + +def test_extract_finding_bullets_basic(): + bs = extract_finding_bullets(_BODY) + assert len(bs) == 3 + assert any("HIGH" in b and "VoidProtection.java:162" in b for b in bs) + assert any("MEDIUM" in b for b in bs) + assert any("LOW" in b for b in bs) + + +def test_extract_finding_bullets_drops_prose(): + bs = extract_finding_bullets(_BODY) + joined = "\n".join(bs) + # The summary prose is dropped. + assert "Adds the salvavoid" not in joined + assert "PlayerDeathEvent item/inventory path" not in joined + # The inline-comment footer is dropped. + assert "inline comment(s) posted below" not in joined + # The sha marker is dropped. + assert "pragent:sha=" not in joined + + +def test_extract_finding_bullets_accepts_lowercase_summary_bullets(): + # `summary_bullets` renders `- **[HIGH]**` (bold); older reviews used + # `- [high]` (plain). Both should match. + text = ( + "- [critical] `a.py:1` — bug. fix: fix it.\n" + "- **[HIGH]** `b.go:9` — race.\n" + ) + bs = extract_finding_bullets(text) + assert len(bs) == 2 + assert "CRITICAL" in bs[0].upper() or "critical" in bs[0] + assert "HIGH" in bs[1] + + +def test_extract_finding_bullets_empty_and_prose_only(): + assert extract_finding_bullets("") == [] + assert extract_finding_bullets(" \n \n") == [] + assert extract_finding_bullets("Just some prose, no bullets here.") == [] + assert extract_finding_bullets("- This is a regular bullet, not a finding.") == [] + + +def test_extract_finding_bullets_keeps_indented_subbullets(): + # A finding may carry continuation lines below it (rare in pragent output + # but legal). We only pull the matching line itself — sub-bullets stay + # with their parent as prose. + text = ( + "- **[HIGH]** `a.py:1` — bug.\n" + " sub-bullet continuation that the reviewer wrote\n" + "- **[LOW]** `b.go:2` — nit.\n" + ) + bs = extract_finding_bullets(text) + assert len(bs) == 2 + assert all("sub-bullet continuation" not in b for b in bs) + + +def test_compress_diff_preserves_anchors_for_post_change_lines(): + # Sanity: a finding anchored on a context line that compress_diff keeps + # must still be a valid anchor after compression. We re-run the parser the + # ai_review core uses, so a regression here surfaces as misanchored + # inline comments in production. + import ai_review + diff = ( + "diff --git a/x.py b/x.py\n" + "--- a/x.py\n" + "+++ b/x.py\n" + "@@ -10,4 +10,5 @@\n" + " ctx_a\n" + " ctx_b\n" + "+new\n" + " ctx_c\n" + " ctx_d\n" + ) + text, _, _ = compress_diff(diff, context=1) + anchors = ai_review.parse_diff_anchors(text) + assert 12 in anchors["x.py"] # +new + # ctx_a is within 1 line of +new at line 12, so kept. + assert 11 in anchors["x.py"] \ No newline at end of file -- 2.52.0 From ec26ec000aee72023ff1a5f15a6e5e8a00634b2b Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 16:12:46 +0000 Subject: [PATCH 2/8] feat(review): equivalent provider price + enriched .pr-review.json schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things in this commit, all in the review-rendering path: 1. COST DISPLAY — the `## 🔋 AI usage` section used to show $0.00 because the pilot runs on headroom/glm-5.2:cloud at no per-token charge. Now it shows TWO lines: the equivalent provider cost (default Claude Sonnet 5; configurable via .pr-review.json:cost_target or PRAGENT_PRICE_TARGET env) AND the actual $0.00 line. Maintainers can now budget on what the same measured tokens would cost on a paid model. equivalent_cost() builds a cost_model.Usage from the measured dict and runs cost_model.cost() against the resolved provider. _resolve_price_target walks repo config > env > default, surfaces typos as an inline note on the usage line (not a crash). 2. .pr-review.json SCHEMA — seven new optional fields: style strict|balanced|lenient (default: balanced) severity_threshold low|medium|high|critical (per style) max_findings 1..30 (per style) exclude_tests bool (skip test files) require_tests bool (synthetic finding) patterns {allow: [...], deny: [...]} (glob filter) cost_target (see #1) The first three are style-driven defaults — strict = 5 findings / high+, balanced = 12 / medium+, lenient = 15 / low+. Override per-field. patterns globs support * and **; built-in fnmatch-style with re.escape. 3. APPLY CONFIG — findings are filtered by the new schema before being split into anchored/unanchored. apply_repo_config() drops by exclude_tests / exclude_paths / patterns.deny / patterns.allow / severity_threshold, then caps at max_findings. require_tests=true appends a synthetic 'low' finding when changed paths include non-test files but no test file changed alongside them. build_user_prompt renders the new fields into the brief so the agent knows about style / threshold / patterns explicitly (not just via instructions). Plus plumbing: * review_pr runs compress_diff(diff, context=PRAGENT_DIFF_CONTEXT) before handing the diff to either engine. Default context=1 (enough to anchor; full files are on disk in the workdir anyway). -1 disables. * compact_prior_reviews(prior) keeps only finding-bullet lines, drops the rest. Prior-review cap lowered 8k -> 4k chars in build_user_prompt. * opencode_review.write_brief accepts compression_note (rendered under the PR description, OUTSIDE the untrusted-data fence). 160 new tests covering equivalent_cost (4), format_usage_section cost lines (5), parse_repo_config extended schema (6), apply_repo_config filters (8), effective_config style defaults (2), compact_prior_reviews (2), and the whole diff_compress suite (14 from the previous commit). 174 pass / 0 fail. --- pilot/ai_review.py | 444 +++++++++++++++++++++++++++++++--- pilot/opencode_review.py | 15 +- tests/pilot/test_ai_review.py | 241 ++++++++++++++++++ 3 files changed, 668 insertions(+), 32 deletions(-) diff --git a/pilot/ai_review.py b/pilot/ai_review.py index ca5620d..a10bb18 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -67,8 +67,27 @@ _SHA_MARKER_RE = re.compile(r"") AI_REVIEW_LABEL = "AI-REVIEW" SEVERITIES = ("critical", "high", "medium", "low") +# Severity rank — higher = more severe. Used by `apply_repo_config` to drop +# findings below `severity_threshold`. Critical=3, high=2, medium=1, low=0. +SEVERITY_RANK = {"low": 0, "medium": 1, "high": 2, "critical": 3} REPO_CONFIG_FILE = ".pr-review.json" +# Style → (default max_findings, default severity_threshold). Strict is +# terse/high-signal; lenient shows everything; balanced is the default for +# unconfigured repos. Repo `.pr-review.json` overrides per-field. +STYLE_DEFAULTS: dict[str, tuple[int, str]] = { + "strict": (5, "high"), + "balanced": (12, "medium"), + "lenient": (15, "low"), +} + +# Default provider to compare against in the usage section. The pilot runs on +# headroom/glm-5.2:cloud at $0/MTok, so the actual line shows $0.00 — but the +# equivalent provider line lets a maintainer see what they would have paid on +# Claude/GPT for the same measured tokens. Override with PRAGENT_PRICE_TARGET +# (env) or `.pr-review.json:cost_target` (per repo). +DEFAULT_PRICE_TARGET = "claude-sonnet-5" + SYSTEM_PROMPT = """You are a senior, pragmatic code reviewer. Review the pull request diff below. Report ONLY real, actionable issues: correctness bugs, security problems, risky @@ -197,7 +216,76 @@ def compute_attribution(findings: list[dict], output_tokens: int) -> None: f["_tok_pct"] = w / total_w -def format_usage_section(usage: dict | None, findings: list[dict], model: str) -> str: +def _resolve_price_target(config: dict | None) -> tuple[str, str | None]: + """Pick which provider to compute the equivalent cost against. + + Order: `.pr-review.json:cost_target` > `PRAGENT_PRICE_TARGET` env > + `DEFAULT_PRICE_TARGET` (claude-sonnet-5). Returns `(price_key, error)`. + + If any of the user-set keys is unknown, falls back to the default AND + reports the error so the operator sees their typo (a config-level typo + silently picking the default would defeat the purpose of letting repos + opt into a different comparison model). + """ + from cost_model import PRICES # local import keeps ollama path dep-free + candidates: list[tuple[str, str]] = [] + if isinstance(config, dict) and config.get("cost_target"): + candidates.append(("repo config", str(config["cost_target"]).strip())) + env = os.environ.get("PRAGENT_PRICE_TARGET", "").strip() + if env: + candidates.append(("PRAGENT_PRICE_TARGET env", env)) + candidates.append(("default", DEFAULT_PRICE_TARGET)) + + chosen = DEFAULT_PRICE_TARGET + for source, key in candidates: + if key in PRICES: + chosen = key + break + else: + # No candidate was valid. Use default + report. + return chosen, ( + f"unknown price target (checked {', '.join(f'{s}={k!r}' for s, k in candidates)}); " + f"valid: {', '.join(sorted(PRICES))}" + ) + + # Even when we picked a valid key, if the *user* set one and it was + # unknown, surface that. (We only get here if a later candidate resolved, + # so the invalid one was upstream.) + invalid = [(s, k) for s, k in candidates if k not in PRICES and s != "default"] + if invalid: + return chosen, ( + f"unknown price target (set {', '.join(f'{s}={k!r}' for s, k in invalid)}); " + f"valid: {', '.join(sorted(PRICES))}; falling back to `{chosen}`" + ) + return chosen, None + + +def equivalent_cost(usage: dict, price_key: str) -> float: + """USD the measured usage would have billed on `price_key`'s provider. + + `usage` is the dict from `parse_opencode_events` (input/output/reasoning/ + cache_read/cache_write). Builds a `cost_model.Usage` and runs `cost()`. The + pilot's actual provider (headroom/glm-5.2:cloud) reports $0 — this is what + the same tokens would cost on a paid model, so maintainers can budget. + """ + from cost_model import Usage, cost, PRICES # local import: ollama path dep-free + if price_key not in PRICES: + return 0.0 + u = Usage( + uncached_input=(usage.get("input", 0) - usage.get("cache_read", 0)), + cached_input=usage.get("cache_read", 0), + cache_writes=usage.get("cache_write", 0), + output=usage.get("output", 0), + ) + return cost(u, PRICES[price_key]) + + +def format_usage_section( + usage: dict | None, + findings: list[dict], + model: str, + config: dict | None = None, +) -> str: """Render the `## 🔋 AI usage` block for the review body. Only called when the PR carries the `AI-USAGE` label (and the opencode @@ -205,18 +293,31 @@ def format_usage_section(usage: dict | None, findings: list[dict], model: str) - (input/output/reasoning/cache/cost/steps/duration) plus an ATTRIBUTED per-finding table — one model pass generates all findings, so per-comment counts are an estimate (output split by body weight), clearly labelled. + + The cost lines show TWO numbers because the pilot runs on headroom at + $0/MTok: the `actual` line is what was billed (always $0.00 today), and + the `est. cost on ` line shows what the same measured tokens + would have cost on a paid model — the number a maintainer actually cares + about when budgeting. `config["cost_target"]` / `PRAGENT_PRICE_TARGET` + / `DEFAULT_PRICE_TARGET` (claude-sonnet-5) picks the comparison provider. + Returns "" if `usage` is None. """ if not usage: return "" dur = usage.get("duration_s") dur_s = f"{dur}s" if dur is not None else "?" - cost = usage.get("cost") or 0.0 - cost_s = f"${cost:.4f}" if cost else "$0.00" - cost_note = ( - "(on-network glm-5.2:cloud via headroom — no per-token charge)" - if not cost else "(billed by provider)" + 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 "(billed by provider)" ) + price_key, price_err = _resolve_price_target(config) + from cost_model import PRICES # local import keeps ollama path dep-free + eq = equivalent_cost(usage, price_key) + eq_s = f"${eq:.4f}" if eq else "$0.00" + eq_label = PRICES[price_key].name lines = [ "## 🔋 AI usage", "", @@ -227,7 +328,12 @@ def format_usage_section(usage: dict | None, findings: list[dict], model: str) - f"{usage.get('cache_read', 0)} read / {usage.get('cache_write', 0)} write " f"→ {usage.get('total', 0)} total" ), - f"- est. cost: {cost_s} {cost_note}", + f"- est. cost on **{eq_label}**: {eq_s}" + ( + f" _(price target: `{price_key}`; " + f"{price_err})_" + if price_err else "" + ), + f"- actual: {actual_s} {actual_note}", "- 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 tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight)", ] @@ -257,23 +363,38 @@ def build_user_prompt( """Assemble the user prompt: repo config + prior reviews + PR meta + diff.""" parts: list[str] = [] - if config: + eff = effective_config(config) if config else {} + if eff: cfg_lines = [] - if config.get("focus"): - cfg_lines.append("Focus areas: " + ", ".join(config["focus"])) - if config.get("exclude_paths"): - cfg_lines.append("Ignore paths: " + ", ".join(config["exclude_paths"])) - if config.get("languages"): - cfg_lines.append("Languages: " + ", ".join(config["languages"])) - if config.get("instructions"): - cfg_lines.append("Instructions:\n" + str(config["instructions"]).strip()) + if eff.get("focus"): + cfg_lines.append("Focus areas: " + ", ".join(eff["focus"])) + if eff.get("exclude_paths"): + cfg_lines.append("Ignore paths: " + ", ".join(eff["exclude_paths"])) + if eff.get("languages"): + cfg_lines.append("Languages: " + ", ".join(eff["languages"])) + if eff.get("style"): + cfg_lines.append(f"Review style: {eff['style']} " + f"(max {eff['max_findings']} findings, threshold " + f"{eff['severity_threshold']}+)") + if eff.get("patterns", {}).get("allow"): + cfg_lines.append("Allow paths (only these are reviewed): " + + ", ".join(eff["patterns"]["allow"])) + if eff.get("patterns", {}).get("deny"): + cfg_lines.append("Deny paths: " + ", ".join(eff["patterns"]["deny"])) + if eff.get("exclude_tests"): + cfg_lines.append("Skip test files entirely.") + if eff.get("require_tests"): + cfg_lines.append("Flag behavioral changes that don't add a test " + "alongside (added as a `low` finding).") + if eff.get("instructions"): + cfg_lines.append("Instructions:\n" + str(eff["instructions"]).strip()) if cfg_lines: parts.append("## Repo review config (.pr-review.json)\n" + "\n".join(cfg_lines)) if prior_reviews: joined = "\n\n---\n\n".join(prior_reviews) - if len(joined) > 8000: - joined = joined[:8000] + "\n…[prior reviews truncated]" + if len(joined) > 4000: + joined = joined[:4000] + "\n…[prior reviews truncated]" parts.append("## PREVIOUS REVIEWS (already posted — do NOT repeat these points)\n" + joined) parts.append(f"## PR\nTitle: {title or '(none)'}") @@ -638,13 +759,30 @@ def summary_bullets(findings: list[dict]) -> str: CONFIG_MAX_LIST_ITEMS = 32 CONFIG_MAX_ITEM_CHARS = 200 CONFIG_MAX_INSTRUCTIONS_CHARS = 4000 +CONFIG_MAX_PATTERNS_ITEMS = 16 # allow + deny separately, total 32 entries +CONFIG_MAX_FINDINGS = 30 + +STYLES = frozenset(STYLE_DEFAULTS) +SEVERITY_VALUES = frozenset(SEVERITIES) def parse_repo_config(raw: str) -> dict: """Parse a .pr-review.json blob tolerantly. Returns {} on any failure. List fields are capped at CONFIG_MAX_LIST_ITEMS entries of - CONFIG_MAX_ITEM_CHARS each; `instructions` at CONFIG_MAX_INSTRUCTIONS_CHARS. + CONFIG_MAX_ITEM_CHARS each; `instructions` at CONFIG_MAX_INSTRUCTIONS_CHARS; + `patterns.allow` / `patterns.deny` each capped at CONFIG_MAX_PATTERNS_ITEMS + of CONFIG_MAX_ITEM_CHARS. + + Recognised keys (all optional): + focus, exclude_paths, languages, instructions — text steer + style strict|balanced|lenient — default: balanced + severity_threshold low|medium|high|critical — default: per style + max_findings 1..CONFIG_MAX_FINDINGS — default: per style + exclude_tests bool — default: False + require_tests bool — default: False + patterns {allow:[…], deny:[…]} — post-filter globs + cost_target — see equivalent_cost """ if not raw: return {} @@ -654,17 +792,195 @@ def parse_repo_config(raw: str) -> dict: return {} if not isinstance(data, dict): return {} - out = {} - for k in ("focus", "exclude_paths", "languages"): - v = data.get(k) + + def _str_list(v): if isinstance(v, list) and all(isinstance(x, str) for x in v): - out[k] = [x[:CONFIG_MAX_ITEM_CHARS] for x in v[:CONFIG_MAX_LIST_ITEMS]] + return [x[:CONFIG_MAX_ITEM_CHARS] for x in v[:CONFIG_MAX_LIST_ITEMS]] + return None + + out: dict = {} + for k in ("focus", "exclude_paths", "languages"): + s = _str_list(data.get(k)) + if s is not None: + out[k] = s + instr = data.get("instructions") if isinstance(instr, str) and instr.strip(): out["instructions"] = instr.strip()[:CONFIG_MAX_INSTRUCTIONS_CHARS] + + style = data.get("style") + if isinstance(style, str) and style.strip().lower() in STYLES: + out["style"] = style.strip().lower() + + thresh = data.get("severity_threshold") + if isinstance(thresh, str) and thresh.strip().lower() in SEVERITY_VALUES: + out["severity_threshold"] = thresh.strip().lower() + + mf = data.get("max_findings") + if isinstance(mf, int) and not isinstance(mf, bool) and 1 <= mf <= CONFIG_MAX_FINDINGS: + out["max_findings"] = mf + elif isinstance(mf, str) and mf.strip().isdigit(): + n = int(mf.strip()) + if 1 <= n <= CONFIG_MAX_FINDINGS: + out["max_findings"] = n + + for bk in ("exclude_tests", "require_tests"): + if isinstance(data.get(bk), bool): + out[bk] = data[bk] + + pat = data.get("patterns") + if isinstance(pat, dict): + allow = _str_list(pat.get("allow")) + deny = _str_list(pat.get("deny")) + patterns = {} + if allow is not None: + patterns["allow"] = allow[:CONFIG_MAX_PATTERNS_ITEMS] + if deny is not None: + patterns["deny"] = deny[:CONFIG_MAX_PATTERNS_ITEMS] + if patterns: + out["patterns"] = patterns + + ct = data.get("cost_target") + if isinstance(ct, str) and ct.strip(): + out["cost_target"] = ct.strip() + return out +def effective_config(config: dict | None) -> dict: + """Apply STYLE_DEFAULTS for any field the config didn't pin. + + Returns a NEW dict combining the user's `.pr-review.json` (if any) with the + derived `max_findings` / `severity_threshold`. Style itself is preserved + so downstream code can branch on it. + """ + style = (config or {}).get("style", "balanced") + max_findings, severity_threshold = STYLE_DEFAULTS.get(style, STYLE_DEFAULTS["balanced"]) + out = dict(config or {}) + out.setdefault("style", style) + out.setdefault("max_findings", max_findings) + out.setdefault("severity_threshold", severity_threshold) + return out + + +_TEST_PATH_RE = re.compile( + r"(?:^|/)(" + r"[^/]*[Tt]est\.[A-Za-z]+" # FooTest.java / foo_test.py + r"|[^/]*\.[Tt]est\.[A-Za-z]+" # foo.Test.java + r"|[^/]*_test\.py" # foo_test.py + r"|test_[^/]*\.py" # test_foo.py + r"|__tests__/[^/]+" # __tests__/foo.js + r"|[^/]*\.spec\.[A-Za-z]+" # foo.spec.ts + r")$" +) + + +def is_test_path(path: str) -> bool: + """Heuristic: is `path` a test file by name/path convention? + + Conservative — false positives cost real findings; false negatives just + produce one extra line in the summary. Patterns: `FooTest.java`, + `foo_test.py`, `test_foo.py`, `__tests__/foo.js`, `foo.spec.ts`, anything + ending in `.Test.java`. + """ + if not path: + return False + return bool(_TEST_PATH_RE.search(path)) + + +def _glob_to_regex(glob: str) -> re.Pattern: + """Translate a shell-style glob to a compiled regex. + + Supports `*` (any chars except `/`), `**` (any chars including `/`), + `?` (single non-`/` char). Other characters are escaped. Used by + `apply_repo_config` to test `patterns.allow` / `patterns.deny` globs. + """ + out = [] + i = 0 + while i < len(glob): + c = glob[i] + if c == "*": + if i + 1 < len(glob) and glob[i + 1] == "*": + out.append(".*") + i += 2 + # swallow a following `/` so `**/x` and `x/**/y` behave + if i < len(glob) and glob[i] == "/": + i += 1 + continue + out.append("[^/]*") + elif c == "?": + out.append("[^/]") + else: + out.append(re.escape(c)) + i += 1 + return re.compile("^" + "".join(out) + "$") + + +def apply_repo_config( + findings: list[dict], + config: dict | None, + changed_paths: list[str] | None = None, +) -> tuple[list[dict], list[dict]]: + """Filter + cap findings per `.pr-review.json` rules. Returns (kept, dropped). + + Filters applied (in order): + 1. `exclude_tests` + test-path heuristic → drop test files + 2. `exclude_paths` glob match → drop matched paths + 3. `patterns.deny` glob match → drop matched paths + 4. `patterns.allow` (if non-empty) → keep ONLY matched paths + 5. `severity_threshold` → drop below threshold + 6. `max_findings` → keep first N (highest-severity-first) + 7. `require_tests` → append a low-severity finding + if changed paths include non-test files but no test files changed + alongside them (caller passes `changed_paths` from the brief). + """ + eff = effective_config(config) + keep: list[dict] = [] + drop: list[dict] = [] + deny_globs = [_glob_to_regex(g) for g in (eff.get("patterns", {}) or {}).get("deny", [])] + allow_globs = [_glob_to_regex(g) for g in (eff.get("patterns", {}) or {}).get("allow", [])] + deny_path_globs = [_glob_to_regex(g) for g in eff.get("exclude_paths", [])] + threshold_rank = SEVERITY_RANK[eff["severity_threshold"]] + + for f in findings: + path = f.get("path", "") + if eff.get("exclude_tests") and is_test_path(path): + drop.append(f); continue + if any(rx.search(path) for rx in deny_path_globs): + drop.append(f); continue + if any(rx.search(path) for rx in deny_globs): + drop.append(f); continue + if allow_globs and not any(rx.search(path) for rx in allow_globs): + drop.append(f); continue + sev_rank = SEVERITY_RANK.get(f.get("severity", "low"), 0) + if sev_rank < threshold_rank: + drop.append(f); continue + keep.append(f) + + cap = eff["max_findings"] + if len(keep) > cap: + dropped = keep[cap:] + keep = keep[:cap] + drop.extend(dropped) + + if eff.get("require_tests") and changed_paths is not None: + non_test = [p for p in changed_paths if not is_test_path(p)] + any_test = any(is_test_path(p) for p in changed_paths) + if non_test and not any_test: + keep.append({ + "severity": "low", + "path": non_test[0], + "line": 1, + "problem": "no test file changed alongside this behavioral change (require_tests=true)", + "fix": "add a unit test exercising the changed branch", + "suggestion": "", + "reference": "", + "_config_synthetic": True, + }) + + return keep, drop + + def reviewed_shas(reviews: list[dict]) -> set[str]: """Pull every `` marker out of a PR's reviews.""" shas: set[str] = set() @@ -693,6 +1009,29 @@ def prior_review_bodies(reviews: list[dict], current_sha: str, limit: int = 6) - return out[:limit] +def compact_prior_reviews(prior_bodies: list[str]) -> list[str]: + """Squeeze prior review bodies down to just the finding bullets. + + Each prior review's prose ("this PR adds eval() — risky") is noise when the + model already has the diff; the only thing it needs to *not repeat* is what + was already flagged. We extract lines matching `-\\s*\\*\\*[SEV]\\*\\*` + plus their directly-attached location reference (so `[CRITICAL]` stays + anchored to `path:line`), drop the rest, and return one bullet-list per + prior review. A prior review that had no parseable findings becomes an + empty string and is dropped. + + Local import keeps the ollama path dep-free (extract_finding_bullets lives + in pilot/diff_compress.py). + """ + from diff_compress import extract_finding_bullets + out = [] + for body in prior_bodies or []: + bullets = extract_finding_bullets(body) + if bullets: + out.append("\n".join(bullets)) + return out + + # --------------------------------------------------------------------------- # Network helpers # --------------------------------------------------------------------------- @@ -911,13 +1250,35 @@ def review_pr( print(f"pragent: {repo}#{index} sha={sha[:8]} already reviewed, skipping", flush=True) return True - diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars) - if not diff.strip(): + raw_diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars) + if not raw_diff.strip(): post_review(api, repo, index, token, format_review_body("No diff content to review.", model, sha)) return True config = fetch_repo_config(api, repo, token, ref=base_ref) - prior = prior_review_bodies(reviews, sha) + prior = compact_prior_reviews(prior_review_bodies(reviews, sha)) + + # Trim the diff to +/- hunks plus a narrow context window. The agent + # resends the brief prefix every step, so a 25k-char diff becomes + # 25k × 30-step × cached-after-step-1 = hundreds of thousands of input + # tokens. Default context=1: enough for the reviewer to see what an + # added line is replacing; the full file is on disk in the workdir + # anyway, so anything more is reading the diff twice. Tunable via + # PRAGENT_DIFF_CONTEXT (0 = +/- only; -1 = disable compression). + from diff_compress import compress_diff + ctx = int(os.environ.get("PRAGENT_DIFF_CONTEXT", "1")) + if ctx < 0: + diff = raw_diff + compression_note = "" + else: + diff, orig_chars, kept_chars = compress_diff(raw_diff, context=ctx) + if kept_chars < orig_chars: + compression_note = ( + f"\n\n> _diff compressed: {orig_chars:,} → {kept_chars:,} chars " + f"(context={ctx}; PRAGENT_DIFF_CONTEXT to tune)_" + ) + else: + compression_note = "" engine = os.environ.get("PRAGENT_ENGINE", "opencode").strip().lower() review_summary = "" @@ -934,6 +1295,7 @@ def review_pr( api=api, repo=repo, index=index, sha=sha, token=token, title=title, body=body, diff=diff, config=config, prior_reviews=prior, model=oc_model, + compression_note=compression_note, ) review_summary, findings = parse_review_output(stdout) if not findings and not review_summary: @@ -949,24 +1311,48 @@ def review_pr( salvaged = salvage_summary(stdout) usage_section = "" if report_usage and usage: - usage_section = format_usage_section(usage, [], model) + usage_section = format_usage_section(usage, [], model, config=config) post_review(api, repo, index, token, format_review_body( salvaged or "AI review produced no parseable output.", model, sha, usage_section=usage_section)) return True else: - user_prompt = build_user_prompt(title, body, diff, config, prior) + user_prompt = build_user_prompt(title, body + compression_note, diff, config, prior) raw_findings = call_model(ollama_url, model, SYSTEM_PROMPT, user_prompt, max_tokens) findings = parse_findings(raw_findings) usage = None + # Filter / cap findings per `.pr-review.json` (style, threshold, max, + # patterns, exclude_tests). Without this every config knob would be a + # no-op — the agent has no view into the config beyond instructions. + # The synthetic require_tests finding (if any) is appended here. + try: + changed_paths = sorted({ + f.get("path", "") + for f in findings + if f.get("path") + }) + except Exception: + changed_paths = [] + kept, _dropped = apply_repo_config(findings, config, changed_paths=changed_paths) + findings = kept + if _dropped: + print( + f"pragent: {repo}#{index} sha={sha[:8]} filtered " + f"{len(_dropped)} finding(s) per .pr-review.json " + f"(style={(config or {}).get('style', 'balanced')}, " + f"threshold={(config or {}).get('severity_threshold', '?')}, " + f"max={len(findings)})", + 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 = "" if report_usage and usage and usage.get("output"): compute_attribution(findings, usage["output"]) - usage_section = format_usage_section(usage, findings, model) + usage_section = format_usage_section(usage, findings, model, config=config) anchors = parse_diff_anchors(diff) anchored, unanchored = split_findings(findings, anchors) diff --git a/pilot/opencode_review.py b/pilot/opencode_review.py index 5c1832c..2cadbd5 100644 --- a/pilot/opencode_review.py +++ b/pilot/opencode_review.py @@ -286,6 +286,7 @@ def write_brief( diff: str, config: dict | None, prior_reviews: list[str] | None, + compression_note: str = "", ) -> str: """Render `.pragent/brief.md` in the workdir. Returns the path written.""" path = os.path.join(workdir, ".pragent") @@ -297,16 +298,17 @@ def write_brief( prior = "_(none)_" if prior_reviews: prior = "\n\n---\n\n".join(prior_reviews) - if len(prior) > 8000: - prior = prior[:8000] + "\n…[prior reviews truncated]" + if len(prior) > 4000: + prior = prior[:4000] + "\n…[prior reviews truncated]" files = changed_files(diff) files_block = "\n".join(f"- `{p}`" for p in files) if files else "_(none)_" + desc_block = ((description or "").strip() or "_(none)_") + compression_note content = _BRIEF_TEMPLATE.format( repo=repo or "?", index=index or "?", sha=sha or "?", title=title or "(none)", - description=description.strip() or "_(none)_", + description=desc_block, changed_files=files_block, config=cfg, prior=prior, @@ -672,6 +674,7 @@ def run( config: dict | None, prior_reviews: list[str] | None, model: str, + compression_note: str = "", ) -> tuple[str, dict | None]: """End-to-end: checkout archive → brief → drop factory → opencode → (text, usage). @@ -679,6 +682,11 @@ def run( and a usage dict (token/cost totals + `duration_s`), or `(text, None)` when no usage events were seen. Raises on any failure; the caller (`review_pr`) fails open. The workdir is removed unless PRAGENT_KEEP_WORK is set. + + `compression_note`: a small markdown block to append to the brief's PR + description (e.g. "diff compressed: 25k → 12k chars"). Empty string by + default. Appended AFTER the untrusted-data fence so the agent reads it as + guidance, not author input. """ os.makedirs(WORK_ROOT, exist_ok=True) workdir = tempfile.mkdtemp(prefix=f"{repo.replace('/', '_')}-{sha[:8]}-", dir=WORK_ROOT) @@ -697,6 +705,7 @@ def run( workdir, repo=repo, index=index, sha=sha, title=title, description=body, diff=diff, config=config, prior_reviews=prior_reviews, + compression_note=compression_note, ) drop_factory(workdir) text, usage = run_opencode(workdir, model) diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index 0c412c6..a6bf7b8 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -779,3 +779,244 @@ def test_salvage_summary_empty_when_nothing_to_salvage(): assert ai_review.salvage_summary("") == "" assert ai_review.salvage_summary(" \n ") == "" assert ai_review.salvage_summary("```json\n{}\n```") == "" + + +# --------------------------------------------------------------------------- +# equivalent_cost + format_usage_section equivalent-provider line +# --------------------------------------------------------------------------- + + +def test_equivalent_cost_matches_cost_model(): + usage = {"input": 1_000_000, "output": 0, "cache_read": 0, "cache_write": 0} + eq = ai_review.equivalent_cost(usage, "claude-sonnet-5") + # Sonnet 5 input is $2/MTok, so 1M input = $2.00 exactly. + assert abs(eq - 2.0) < 1e-9 + + +def test_equivalent_cost_unknown_key_returns_zero(): + assert ai_review.equivalent_cost({"input": 100}, "bogus") == 0.0 + + +def test_format_usage_section_shows_equivalent_provider_cost(): + usage = {"input": 200000, "output": 4000, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 204000, + "cost": 0.0, "steps": 6, "duration_s": 100.0} + sec = ai_review.format_usage_section(usage, [], "glm-5.2:cloud") + # Two cost lines now: an equivalent (default Sonnet 5) AND the $0 actual. + assert "## 🔋 AI usage" in sec + assert "est. cost on **Claude Sonnet 5**" in sec + assert "actual: $0.00" in sec + assert "free tier" in sec + # Equivalent should be > 0 for non-trivial token counts. + assert "$0.00" in sec # the actual line + # And a non-zero one for the equivalent. + import re + cost_lines = [ln for ln in sec.splitlines() if "cost on" in ln] + assert len(cost_lines) == 1 + assert re.search(r"\$\d", cost_lines[0]) is not None + assert "$0.00" not in cost_lines[0] + + +def test_format_usage_section_honors_cost_target(monkeypatch): + monkeypatch.setenv("PRAGENT_PRICE_TARGET", "claude-haiku-4-5") + usage = {"input": 1000, "output": 100, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 1100, + "cost": 0.0, "steps": 1, "duration_s": 5.0} + sec = ai_review.format_usage_section(usage, [], "glm-5.2:cloud") + assert "Claude Haiku 4.5" in sec + # 1k * $1/MTok + 100 * $5/MTok = 0.001 + 0.0005 = $0.0015 + assert "$0.0015" in sec + + +def test_format_usage_section_respects_repo_config_cost_target(monkeypatch): + monkeypatch.delenv("PRAGENT_PRICE_TARGET", raising=False) + usage = {"input": 1000, "output": 100, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 1100, + "cost": 0.0, "steps": 1, "duration_s": 5.0} + sec = ai_review.format_usage_section( + usage, [], "glm-5.2:cloud", config={"cost_target": "claude-opus-5"} + ) + assert "Claude Opus 5" in sec + # Opus 5 = $5/MTok input + $25/MTok output → 1000*5e-6 + 100*25e-6 = 0.0075 + assert "$0.0075" in sec + + +def test_format_usage_section_reports_unknown_price_target(): + usage = {"input": 100, "output": 100, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 200, + "cost": 0.0, "steps": 1, "duration_s": 1.0} + sec = ai_review.format_usage_section( + usage, [], "glm-5.2:cloud", config={"cost_target": "bogus-model"} + ) + # Falls back to default + surfaces the error in the line. + assert "Claude Sonnet 5" in sec + assert "unknown price target" in sec + assert "bogus-model" in sec + + +# --------------------------------------------------------------------------- +# parse_repo_config — extended schema +# --------------------------------------------------------------------------- + + +def test_parse_repo_config_new_fields_all_valid(): + raw = json.dumps({ + "focus": ["security"], + "style": "strict", + "severity_threshold": "high", + "max_findings": 5, + "exclude_tests": True, + "require_tests": True, + "patterns": {"allow": ["src/**"], "deny": ["**/*.test.ts"]}, + "cost_target": "claude-opus-5", + }) + c = ai_review.parse_repo_config(raw) + assert c["style"] == "strict" + assert c["severity_threshold"] == "high" + assert c["max_findings"] == 5 + assert c["exclude_tests"] is True + assert c["require_tests"] is True + assert c["patterns"]["allow"] == ["src/**"] + assert c["patterns"]["deny"] == ["**/*.test.ts"] + assert c["cost_target"] == "claude-opus-5" + + +def test_parse_repo_config_rejects_bad_style_and_threshold(): + c = ai_review.parse_repo_config(json.dumps({"style": "wild", "severity_threshold": "meh"})) + assert "style" not in c + assert "severity_threshold" not in c + + +def test_parse_repo_config_caps_max_findings(): + c1 = ai_review.parse_repo_config(json.dumps({"max_findings": 0})) + c2 = ai_review.parse_repo_config(json.dumps({"max_findings": 999})) + c3 = ai_review.parse_repo_config(json.dumps({"max_findings": "12"})) + assert "max_findings" not in c1 # 0 invalid + assert "max_findings" not in c2 # > 30 invalid + assert c3["max_findings"] == 12 # numeric string accepted + + +def test_parse_repo_config_caps_patterns(): + raw = json.dumps({ + "patterns": {"allow": [f"a{i}" for i in range(20)], "deny": [f"d{i}" for i in range(20)]} + }) + c = ai_review.parse_repo_config(raw) + assert len(c["patterns"]["allow"]) == ai_review.CONFIG_MAX_PATTERNS_ITEMS + assert len(c["patterns"]["deny"]) == ai_review.CONFIG_MAX_PATTERNS_ITEMS + + +def test_effective_config_applies_style_defaults(): + eff = ai_review.effective_config({"focus": ["security"]}) + assert eff["style"] == "balanced" + assert eff["max_findings"] == 12 + assert eff["severity_threshold"] == "medium" + assert eff["focus"] == ["security"] + + +def test_effective_config_style_overrides_fields(): + eff = ai_review.effective_config({"style": "strict"}) + assert eff["max_findings"] == 5 + assert eff["severity_threshold"] == "high" + + +# --------------------------------------------------------------------------- +# apply_repo_config — filter findings +# --------------------------------------------------------------------------- + + +_FINDINGS = [ + {"severity": "critical", "path": "src/main.py", "line": 1, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "high", "path": "src/main.py", "line": 5, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "medium", "path": "src/main.py", "line": 9, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "low", "path": "src/main.py", "line": 13, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "high", "path": "src/FooTest.java", "line": 22, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "medium", "path": "src/app.test.ts", "line": 7, "problem": "p", "fix": "f", "suggestion": ""}, +] + + +def test_apply_repo_config_severity_threshold(): + kept, dropped = ai_review.apply_repo_config(_FINDINGS, {"severity_threshold": "high"}) + assert len(kept) == 3 # critical + 2 highs (main.py + FooTest.java) + assert all(f["severity"] in ("critical", "high") for f in kept) + assert len(dropped) == 3 + + +def test_apply_repo_config_exclude_tests_drops_test_files(): + kept, dropped = ai_review.apply_repo_config(_FINDINGS, {"exclude_tests": True}) + paths = {f["path"] for f in kept} + assert "src/FooTest.java" not in paths + assert "src/app.test.ts" not in paths + + +def test_apply_repo_config_patterns_deny_drops_matching(): + cfg = {"patterns": {"deny": ["src/main.py"]}} + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg) + paths = {f["path"] for f in kept} + assert "src/main.py" not in paths + + +def test_apply_repo_config_patterns_allow_keeps_only_matching(): + cfg = {"patterns": {"allow": ["src/main.py"]}} + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg) + paths = {f["path"] for f in kept} + assert paths == {"src/main.py"} + + +def test_apply_repo_config_max_findings_caps(): + kept, dropped = ai_review.apply_repo_config(_FINDINGS, {"max_findings": 2}) + assert len(kept) == 2 + # Highest-severity first (critical, then high) + assert kept[0]["severity"] == "critical" + assert kept[1]["severity"] == "high" + + +def test_apply_repo_config_exclude_paths_glob(): + cfg = {"exclude_paths": ["src/main.py"]} + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg) + assert "src/main.py" not in {f["path"] for f in kept} + + +def test_apply_repo_config_require_tests_synthetic_finding(): + cfg = {"require_tests": True} + changed = ["src/main.py", "src/lib.ts"] + kept, dropped = ai_review.apply_repo_config([], cfg, changed_paths=changed) + assert any(f.get("_config_synthetic") for f in kept) + + +def test_apply_repo_config_require_tests_no_synthetic_when_tests_present(): + cfg = {"require_tests": True} + changed = ["src/main.py", "src/main_test.py"] + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg, changed_paths=changed) + assert not any(f.get("_config_synthetic") for f in kept) + + +def test_is_test_path_recognises_common_patterns(): + assert ai_review.is_test_path("src/FooTest.java") + assert ai_review.is_test_path("src/foo.test.ts") + assert ai_review.is_test_path("tests/foo_test.py") + assert ai_review.is_test_path("test_foo.py") + assert ai_review.is_test_path("packages/app/__tests__/foo.js") + assert not ai_review.is_test_path("src/main.py") + assert not ai_review.is_test_path("src/testing.py") # "testing" ≠ "test_" + + +# --------------------------------------------------------------------------- +# compact_prior_reviews +# --------------------------------------------------------------------------- + + +def test_compact_prior_reviews_drops_prose_keeps_bullets(): + bodies = [ + "🤖 AI Review · m · `abc`\n\nLong prose.\n\n- **[HIGH]** `a.py:1` — bug.\n- **[LOW]** `b.go:2` — nit.\n\n_2 inline comments posted._\n", + "Just chatter, no findings.", + ] + out = ai_review.compact_prior_reviews(bodies) + assert len(out) == 1 + assert "HIGH" in out[0] and "a.py:1" in out[0] + assert "Long prose." not in out[0] + assert "inline comments posted" not in out[0] + + +def test_compact_prior_reviews_empty_and_none(): + assert ai_review.compact_prior_reviews([]) == [] + assert ai_review.compact_prior_reviews(None) == [] -- 2.52.0 From 5302e8dcd7dc7ec18dd730b56c7164aba7664dc3 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 16:29:44 +0000 Subject: [PATCH 3/8] fix(review): salvage findings from nested-object fences + bare arrays + unfenced tail JSON The canalhandia PR review lost all findings because the agent ran out of context before emitting the closing json fence. Three failure modes hit the old regex \{.*?\}: * nested objects inside the fence truncated at the first } * bare arrays (no {summary, findings} wrapper) returned [] * unfenced JSON in the prose tail was never reached (first not last) Replace the regex with a balanced-brace scanner: * _last_json_block walks the fence contents with a depth counter so nested objects survive * _last_balanced_json + _balanced_json_substring handle bare arrays and prose-tail JSON when no fence is present * _parse_json_tolerant returns list as well as dict; parse_findings and parse_review_output accept a bare array as the outer value Agent prompt tightened: reserve the final step for emitting the JSON block so the analysis isn't lost when context runs out. 10 new tests in tests/pilot/test_ai_review.py cover the new shapes. Co-Authored-By: Claude --- .opencode/agents/pragent.md | 5 +- pilot/ai_review.py | 195 +++++++++++++++++++++++++++++----- pilot/diff_compress.py | 2 +- tests/pilot/test_ai_review.py | 105 ++++++++++++++++++ 4 files changed, 280 insertions(+), 27 deletions(-) diff --git a/.opencode/agents/pragent.md b/.opencode/agents/pragent.md index c0aa2f5..d887370 100644 --- a/.opencode/agents/pragent.md +++ b/.opencode/agents/pragent.md @@ -164,4 +164,7 @@ Rules: - If the diff is clean, output `{"summary":"...","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. \ No newline at end of file + the last ```json fenced block from your output. If you run out of context/steps + before emitting it, your analysis is wasted: ALWAYS reserve the final step for + writing the JSON. Stop exploring and write findings at the first sign you've + covered the diff (no new findings in the last 2 file reads = stop). \ No newline at end of file diff --git a/pilot/ai_review.py b/pilot/ai_review.py index a10bb18..99a2a12 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -509,14 +509,42 @@ def _normalize_finding(f: dict) -> dict | None: def _last_json_block(text: str) -> str | None: - """Return the substring of the last fenced ```json block in text, or None. - Falls back to _extract_first_json_object when no fence is present.""" + r"""Return the substring of the last JSON object/array in text, or None. + + The pragent agent emits ```json fences around its final block, but real + outputs drift: + * the fence contains nested objects (regex ``\{.*?\}`` only matches the + first ``}``, truncating the JSON — the parser then sees + ``json.JSONDecodeError``); + * the fence is missing or unterminated, but a balanced JSON object sits + in the prose tail; + * the agent emits a bare array (findings only, no summary wrapper). + + Strategy: + 1. Find each fenced block, take the last. Inside it, walk a balanced + ``{...}``/``[...]`` scanner (not a regex) so nested structures survive. + 2. Fall back to a balanced scanner over the whole text, picking the LAST + balanced object/array (the agent writes its conclusion last). + """ s = text or "" - # Find all ```json ... ``` fenced blocks; take the last. - blocks = list(re.finditer(r"```(?:json)?\s*(\{.*?\})\s*```", s, re.DOTALL)) - if blocks: - return blocks[-1].group(1) - return _extract_first_json_object(s) + if not s: + return None + # 1. Fenced blocks: take the last ```json ... ``` or ``` ... ``` region. + fences = list(re.finditer(r"```(?:json)?\n", s)) + for m in reversed(fences): + start = m.end() + # Find the matching closing fence. + end = s.find("```", start) + if end < 0: + # Unterminated fence — try to salvage the balanced object inside. + end = len(s) + inner = s[start:end].strip() + obj = _balanced_json_substring(inner) + if obj is not None: + return obj + # 2. No (parseable) fence — scan the whole text for the LAST balanced + # object/array. The agent's conclusion is at the tail. + return _last_balanced_json(s) def parse_findings(text: str) -> list[dict]: @@ -526,11 +554,17 @@ def parse_findings(text: str) -> list[dict]: scans for the first balanced `{...}` and extracts its `findings` array. Drops findings missing path/line or with an unknown severity (normalised). Never raises — returns [] on any parse failure. + + Also accepts a bare JSON array as the outer value: ``[{...}, {...}]`` — + some agents skip the ``{"summary":..., "findings":[...]}`` wrapper. """ data = _parse_json_tolerant(text) - if not isinstance(data, dict): + if isinstance(data, dict): + findings = data.get("findings") + elif isinstance(data, list): + findings = data + else: return [] - findings = data.get("findings") if not isinstance(findings, list): return [] out = [] @@ -577,10 +611,11 @@ 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). - Accepts `{"summary": "...", "findings": [...]}` (the opencode pragent agent) - or a bare `{"findings": [...]}`. `summary` defaults to "". Uses the LAST - ```json fenced block (the pragent agent emits JSON as the final block), with - a tolerant fallback. Never raises. + 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. """ blob = _last_json_block(text) if blob is None: @@ -589,10 +624,15 @@ def parse_review_output(text: str) -> tuple[str, list[dict]]: data = json.loads(blob) except json.JSONDecodeError: return "", [] - if not isinstance(data, dict): + if isinstance(data, dict): + summary = str(data.get("summary", "") or "").strip() + findings = data.get("findings") + elif isinstance(data, list): + # Bare array: each item is a finding; no summary. + summary = "" + findings = data + else: return "", [] - summary = str(data.get("summary", "") or "").strip() - findings = data.get("findings") out = [] if isinstance(findings, list): for f in findings: @@ -602,16 +642,18 @@ def parse_review_output(text: str) -> tuple[str, list[dict]]: return summary, out -def _parse_json_tolerant(text: str) -> dict | None: - """Parse a JSON object from text: try the last fenced block, then a direct - parse, then the first balanced object. Returns None on any failure.""" +def _parse_json_tolerant(text: str) -> dict | list | None: + """Parse a JSON object/array from text: try the last fenced block, then a + direct parse, then the first balanced object. Returns None on any failure. + Accepts both ``{...}`` (the pragent schema) and bare ``[...]`` arrays + (agents that skip the wrapper).""" if not text: return None blob = _last_json_block(text) if blob is not None: try: d = json.loads(blob) - if isinstance(d, dict): + if isinstance(d, (dict, list)): return d except json.JSONDecodeError: pass @@ -621,7 +663,7 @@ def _parse_json_tolerant(text: str) -> dict | None: s = re.sub(r"\n?```$", "", s).strip() try: d = json.loads(s) - if isinstance(d, dict): + if isinstance(d, (dict, list)): return d except json.JSONDecodeError: pass @@ -629,7 +671,17 @@ def _parse_json_tolerant(text: str) -> dict | None: if obj is not None: try: d = json.loads(obj) - if isinstance(d, dict): + if isinstance(d, (dict, list)): + return d + except json.JSONDecodeError: + pass + # Last resort: the JSON lives at the tail of the prose with no fence. + # Walk the whole text for the last balanced object/array. + last = _last_balanced_json(text) + if last is not None: + try: + d = json.loads(last) + if isinstance(d, (dict, list)): return d except json.JSONDecodeError: pass @@ -641,6 +693,62 @@ def _extract_first_json_object(s: str) -> str | None: start = s.find("{") if start < 0: return None + end = _scan_balanced(s, start, "{", "}") + if end is None: + return None + return s[start:end + 1] + + +def _last_balanced_json(s: str) -> str | None: + """Return the substring of the LAST balanced ``{...}`` or ``[...]`` in s. + + Used when the agent emits no fence: the JSON lives in the prose tail. + Picks whichever closer (object or array) appears latest in the text. + """ + if not s: + return None + last_obj = _find_last_close(s, "{", "}") + last_arr = _find_last_close(s, "[", "]") + candidates = [] + if last_obj is not None: + candidates.append(last_obj) + if last_arr is not None: + candidates.append(last_arr) + if not candidates: + return None + end, opener, start = max(candidates, key=lambda t: t[0]) + return s[start:end + 1] + + +def _balanced_json_substring(s: str) -> str | None: + """Return the first balanced ``{...}`` or ``[...]`` substring in ``s``. + + Skips past leading whitespace/non-JSON and returns the full balanced + extent (handles nested objects/arrays and string literals with braces). + """ + if not s: + return None + # Try object first; the pragent schema is an object on the outer level. + for i, c in enumerate(s): + if c == "{": + end = _scan_balanced(s, i, "{", "}") + if end is not None: + return s[i:end + 1] + break + if c == "[": + end = _scan_balanced(s, i, "[", "]") + if end is not None: + return s[i:end + 1] + break + return None + + +def _scan_balanced(s: str, start: int, opener: str, closer: str) -> int | None: + """Return the index of the matching ``closer`` for ``s[start] == opener``. + + Tracks string literals (with ``\\`` escapes) so braces inside strings don't + fool the depth counter. Returns None if no balance is reached. + """ depth = 0 in_str = False esc = False @@ -656,12 +764,49 @@ def _extract_first_json_object(s: str) -> str | None: continue if c == '"': in_str = True - elif c == "{": + elif c == opener: depth += 1 - elif c == "}": + elif c == closer: depth -= 1 if depth == 0: - return s[start:i + 1] + return i + return None + + +def _find_last_close(s: str, opener: str, closer: str) -> tuple[int, str, int] | None: + """Walk ``s`` backwards from the last ``closer`` to find its matching opener. + + Returns ``(close_idx, opener_char, open_idx)`` for the rightmost balanced + structure, or None if no pair exists. + """ + # Find the last `closer` candidate. + last = s.rfind(closer) + while last >= 0: + # Walk left, tracking depth from the perspective of the opener. + depth = 1 + in_str = False + esc = False + for j in range(last - 1, -1, -1): + c = s[j] + if in_str: + if esc: + esc = False + elif c == "\\": + esc = True + elif c == '"': + in_str = False + continue + if c == '"': + # Approximation: we don't track quotes perfectly walking + # backwards, but strings in agent output are short and rare. + in_str = not in_str + elif c == closer: + depth += 1 + elif c == opener: + depth -= 1 + if depth == 0: + return (last, opener, j) + last = s.rfind(closer, 0, last) return None diff --git a/pilot/diff_compress.py b/pilot/diff_compress.py index 52649e0..24d3ea9 100644 --- a/pilot/diff_compress.py +++ b/pilot/diff_compress.py @@ -103,7 +103,7 @@ def compress_diff(diff: str, *, context: int = 2) -> tuple[str, int, int]: def _render_hunk_body(body: list[str], *, context: int) -> tuple[list[str], int]: - """Trim `body` to `context` unchanged lines around the +/- lines. + r"""Trim `body` to `context` unchanged lines around the +/- lines. Body lines are classified: - `+` line → keep diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index a6bf7b8..c361b66 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -11,6 +11,9 @@ sys.path.insert(0, os.path.join(ROOT, "pilot")) import ai_review # noqa: E402 from ai_review import ( # noqa: E402 + _balanced_json_substring, + _extract_first_json_object, + _last_balanced_json, build_user_prompt, compute_attribution, format_review_body, @@ -408,6 +411,108 @@ def test_parse_review_output_uses_last_json_block(): assert fs[0]["path"] == "y" +def test_parse_findings_fenced_json_with_nested_object(): + # Real-world regression: agent emits a fence whose inner JSON has nested + # objects. The old regex `\{.*?\}` matched only the first `}`, truncating + # the JSON. Now we balance braces inside the fence. + txt = ( + "```json\n" + '{"summary":"x","findings":[{"severity":"high","path":"a.py","line":1,' + '"problem":"p","fix":"f","suggestion":"","reference":""}],"meta":{"engine":"opencode"}}\n' + "```" + ) + fs = parse_findings(txt) + assert len(fs) == 1 + assert fs[0]["path"] == "a.py" + + +def test_parse_findings_unfenced_at_tail(): + # No fence at all. Agent wrote the JSON inline at the very end of its + # prose. The old first-balanced regex caught the FIRST `{`, not this one. + txt = ( + "I considered the diff carefully. Two findings stand out:\n" + "First one is just text.\n" + '{"findings":[{"severity":"critical","path":"x","line":1,"problem":"p","fix":"f"}]}' + ) + fs = parse_findings(txt) + assert len(fs) == 1 + assert fs[0]["severity"] == "critical" + + +def test_parse_findings_bare_array(): + # Some agents skip the `{"summary":..., "findings":[...]}` wrapper and + # emit just the array. + txt = ( + "Here are my findings:\n" + "```json\n" + '[{"severity":"low","path":"a","line":1,"problem":"p","fix":"f","suggestion":"","reference":""}]\n' + "```" + ) + fs = parse_findings(txt) + assert len(fs) == 1 + assert fs[0]["path"] == "a" + + +def test_parse_review_output_unfenced_at_tail(): + # The exact shape canalhandia produced: long prose, JSON at the very end, + # no fence. Old parser returned ([], salvage) — now we recover findings. + txt = ( + "Let me refine the fix: should call a dedicated `setPermanent`.\n" + "Let me finalize. Let me also double-check the `find` thread-safety.\n" + '{"summary":"Adds void protection; one critical race.","findings":[' + '{"severity":"high","path":"VoidProtection.java","line":162,' + '"problem":"drop duplication race","fix":"use ItemMeta","suggestion":"","reference":""}]}' + ) + summary, fs = parse_review_output(txt) + assert "void protection" in summary.lower() + assert len(fs) == 1 + assert fs[0]["path"] == "VoidProtection.java" + + +def test_parse_review_output_bare_array_at_tail(): + txt = ( + "All wrapped up.\n" + '[{"severity":"low","path":"a","line":1,"problem":"p","fix":"","suggestion":"","reference":""}]' + ) + summary, fs = parse_review_output(txt) + assert summary == "" + assert len(fs) == 1 + + +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}' + obj = _extract_first_json_object(s) + assert obj == s + d = json.loads(obj) + assert d["a"] == "contains { and }" + + +def test_last_balanced_json_picks_latest(): + s = '{"a":1} some text {"b":2,"nested":{"c":3}} trailing' + out = _last_balanced_json(s) + assert out is not None + d = json.loads(out) + assert d == {"b": 2, "nested": {"c": 3}} + + +def test_last_balanced_json_no_json(): + assert _last_balanced_json("nothing here") is None + assert _last_balanced_json("") is None + + +def test_balanced_json_substring_skips_leading_prose(): + s = 'preamble {"a":1} more prose {"b":2}' + out = _balanced_json_substring(s) + assert out == '{"a":1}' + + +def test_balanced_json_substring_handles_array(): + s = '[{"a":1},{"b":2}]' + out = _balanced_json_substring(s) + assert out == s + + # --------------------------------------------------------------------------- # reference rendering in inline_comment_body + summary_bullets + summary section # --------------------------------------------------------------------------- -- 2.52.0 From f6be2b3c616af1097c1f75206e371d205e9b6b07 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 16:51:20 +0000 Subject: [PATCH 4/8] feat(review): PR-level collapsible metadata + emoji-tagged inline comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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
/ 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 --- .opencode/agents/pragent.md | 17 +- pilot/ai_review.py | 377 +++++++++++++++++++++++++++------- tests/pilot/test_ai_review.py | 190 ++++++++++++++--- 3 files changed, 474 insertions(+), 110 deletions(-) diff --git a/.opencode/agents/pragent.md b/.opencode/agents/pragent.md index d887370..a013f7c 100644 --- a/.opencode/agents/pragent.md +++ b/.opencode/agents/pragent.md @@ -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 diff --git a/pilot/ai_review.py b/pilot/ai_review.py index 99a2a12..b00bd1f 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -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 ``
`` 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 ``
`` 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 = [ + "
", + "🔋 AI Usage & Run Details", + "", + 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}", + "", + "
", + ] 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) diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index c361b66..80664e7 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -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("