fix(review): correct diff-compression line numbers, prior-review dedupe, triage skip
Four defects, all found reviewing PR #9 (two of them by pragent-bot's own review of that PR, which the anchoring bug then misplaced): * compress_diff dropped context lines but copied the original `@@` hunk header verbatim, so the header no longer described the lines beneath it. parse_diff_anchors then walked stale headers and produced anchor sets shifted by the number of elided lines, misplacing inline comments or demoting them to bullets. Each surviving run of lines is now re-emitted as its own hunk with a recomputed `@@ -a,b +c,d @@`, so the output stays a valid unified diff whose numbers describe the real post-change file. The pseudo-marker `@@ … N context line(s) omitted … @@` is gone; it parsed as a hunk header and reset the anchor counter to 0. Anchoring additionally runs on the raw diff now, so the prompt window can never shrink the anchorable set. * compress_diff's `_FILE_HEADER` regex matched diff *body* lines: a removed YAML `---` separator or an added `++` line was read as a file header, truncating the hunk and dropping its `@@` header with it. Body detection is now prefix-based, with a full-shape hunk-header regex. * extract_finding_bullets could not match the bullets pragent itself posts: summary_bullets renders an emoji severity badge between the `-` and the `[SEV]` tag, which the regex rejected, so compact_prior_reviews always returned [] and every re-review repeated its previous findings. * triage returning `{"lenses":[]}` — documented in .opencode/agents/triage.md as "no lens has surface, skip the fan-out" — ran every lens instead, since _intersect_with_triage mapped an empty selection to "all" and the call site had a second `or reviewers` fallback. `[]` and None are now distinct outcomes: `[]` skips, None fails open. A roster naming only unknown lens ids now fails open rather than silencing the review. The skip path returns a well-formed empty-findings response instead of "", which had landed in ai_review's unparseable-output branch and posted "AI review produced no parseable output" — a malfunction message for a normal verdict. Also: non-URL references (a CVE id, a doc title) rendered as `[CVE-2024-1234](CVE-2024-1234)`, a broken relative link in Gitea — now plain text. PRAGENT_DIFF_CONTEXT and friends parse through _int_env, so a typo logs and falls back instead of killing a review mid-flight. Removed format_usage_section, dead since the collapsible usage block replaced it and carrying a duplicate copy of the price-target logic. Tests: 290 -> 301. New coverage for hunk-header fidelity before/after compression, header-shaped content lines, the bullet round-trip against the real renderer, and triage's three outcomes (previously untested). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B11e8TZZxJyzHW7jj7KWUN
This commit is contained in:
+32
-79
@@ -156,6 +156,25 @@ def parse_text_blocks(content: list) -> str:
|
||||
return "\n".join(out).strip()
|
||||
|
||||
|
||||
def _int_env(name: str, default: int) -> int:
|
||||
"""Read an int from the environment, falling back on anything unparseable.
|
||||
|
||||
A typo in a tuning knob must not take down a review that is already
|
||||
mid-flight — the operator gets a stderr line and the default instead.
|
||||
"""
|
||||
raw = os.environ.get(name, "")
|
||||
if not str(raw).strip():
|
||||
return default
|
||||
try:
|
||||
return int(str(raw).strip())
|
||||
except (TypeError, ValueError):
|
||||
print(
|
||||
f"pragent: ignoring {name}={raw!r} (not an integer); using {default}",
|
||||
file=sys.stderr, flush=True,
|
||||
)
|
||||
return default
|
||||
|
||||
|
||||
def format_review_body(
|
||||
findings: str,
|
||||
model: str,
|
||||
@@ -337,79 +356,6 @@ def equivalent_cost(usage: dict, price_key: str) -> float:
|
||||
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
|
||||
engine produced a usage dict). Reports the MEASURED total
|
||||
(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 <provider>` 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 "?"
|
||||
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",
|
||||
"",
|
||||
f"- model: `{model}` · engine: opencode · agent steps: {usage.get('steps', 0)} · duration: {dur_s}",
|
||||
(
|
||||
f"- tokens: {usage.get('input', 0)} in · {usage.get('output', 0)} out · "
|
||||
f"{usage.get('reasoning', 0)} reasoning · cache "
|
||||
f"{usage.get('cache_read', 0)} read / {usage.get('cache_write', 0)} write "
|
||||
f"→ {usage.get('total', 0)} total"
|
||||
),
|
||||
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)",
|
||||
]
|
||||
# Per-finding attribution table.
|
||||
rows = [f for f in findings if f.get("_tok_attrib") is not None]
|
||||
if rows:
|
||||
lines.append("")
|
||||
lines.append("| severity | location | ≈out tok | % |")
|
||||
lines.append("|---|---|---:|---:|")
|
||||
for f in rows:
|
||||
loc = f"{f['path']}:{f['line']}" if f.get("line") else f.get("path", "?")
|
||||
pct = f.get("_tok_pct", 0.0) * 100
|
||||
lines.append(
|
||||
f"| {f.get('severity', '').upper()} | `{loc}` | "
|
||||
f"{f.get('_tok_attrib', 0)} | {pct:.0f}% |"
|
||||
)
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
def build_user_prompt(
|
||||
title: str,
|
||||
body: str,
|
||||
@@ -987,8 +933,10 @@ def _format_reference(ref: str) -> str:
|
||||
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})"
|
||||
# Non-URL text (e.g. a CVE id, a doc title). Render as plain text —
|
||||
# `[CVE-2024-1](CVE-2024-1)` would render as a broken *relative* link
|
||||
# in Gitea, which is worse than no link at all.
|
||||
return ref
|
||||
# Strip the scheme + www. for the visible label so the link text is short.
|
||||
visible = ref
|
||||
for prefix in ("https://", "http://"):
|
||||
@@ -1884,7 +1832,7 @@ def review_pr(
|
||||
# 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"))
|
||||
ctx = _int_env("PRAGENT_DIFF_CONTEXT", 1)
|
||||
if ctx < 0:
|
||||
diff = raw_diff
|
||||
compression_note = ""
|
||||
@@ -1990,7 +1938,12 @@ def review_pr(
|
||||
compute_attribution(findings, usage["output"])
|
||||
usage_section = _render_collapsible_usage(usage, model, config=config) if report_usage else ""
|
||||
|
||||
anchors = parse_diff_anchors(diff)
|
||||
# Anchor against the RAW diff, never the compressed one. Compression
|
||||
# drops context lines, so a finding on a line that survived in the file
|
||||
# but not in the prompt would be demoted to a bullet for no reason.
|
||||
# (compress_diff renumbers its hunks, so both are line-accurate; the
|
||||
# raw diff is simply the complete set.)
|
||||
anchors = parse_diff_anchors(raw_diff)
|
||||
anchored, unanchored = split_findings(findings, anchors)
|
||||
|
||||
# Summary body: unanchored bullets fall through to a "Unanchored notes"
|
||||
@@ -2038,8 +1991,8 @@ def run() -> int:
|
||||
token=_need("PRAGENT_BOT_TOKEN"),
|
||||
ollama_url=_need("OLLAMA_URL"),
|
||||
model=_need("OLLAMA_MODEL"),
|
||||
max_tokens=int(os.environ.get("OLLAMA_MAX_TOKENS", "8000")),
|
||||
max_chars=int(os.environ.get("DIFF_MAX_CHARS", "150000")),
|
||||
max_tokens=_int_env("OLLAMA_MAX_TOKENS", 8000),
|
||||
max_chars=_int_env("DIFF_MAX_CHARS", 150000),
|
||||
base_ref=os.environ.get("PR_BASE_REF", ""),
|
||||
)
|
||||
return 0
|
||||
|
||||
+176
-91
@@ -11,11 +11,18 @@ signal:
|
||||
statement. Wider context = more reading; narrower = less. Set
|
||||
``context=0`` for +/- only, ``context=-1`` to disable entirely.
|
||||
|
||||
Elided context is not merely deleted: each surviving run of lines is
|
||||
re-emitted as its *own* ``@@ -a,b +c,d @@`` hunk with recomputed line
|
||||
numbers, so the output stays a valid unified diff whose line numbers
|
||||
still describe the post-change file. ``parse_diff_anchors`` (and the
|
||||
model) therefore read the same line numbers before and after compression.
|
||||
|
||||
* ``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.
|
||||
review that look like a pragent finding (``- 🔴 [HIGH] `path:line` — …``,
|
||||
or the older ``- **[HIGH]** …`` form) 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.
|
||||
"""
|
||||
@@ -24,20 +31,22 @@ 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:|---|\+\+\+|@@)")
|
||||
# A real hunk header: `@@ -old[,count] +new[,count] @@[ trailing section]`.
|
||||
# Captures both starts, both counts, and the trailing function-context text.
|
||||
# Matching the full shape (not just a `@@` prefix) matters: a *removed* line
|
||||
# whose content begins with `@@` is body, not a header.
|
||||
_HUNK_RE = re.compile(
|
||||
r"^@@\s+-(\d+)(?:,(\d+))?\s+\+(\d+)(?:,(\d+))?\s+@@(.*)$"
|
||||
)
|
||||
|
||||
# Captures `- <n>[,<m>]` AND `+ <n>[,<m>]` 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.
|
||||
# Match a pragent summary-bullet line, in any of the shapes the renderer has
|
||||
# emitted: `- 🔴 [HIGH] \`path:line\` — …` (current, `_severity_badge`),
|
||||
# `- **[HIGH]** …` (bold, pre-badge), `- [high] …` (plain, oldest).
|
||||
# Anything between the bullet marker and `[SEV]` (emoji, bold markers,
|
||||
# whitespace) is tolerated — it is decoration, not signal.
|
||||
_FINDING_BULLET_RE = re.compile(
|
||||
r"^\s*-\s*\*?\*?\[(?P<sev>critical|high|medium|low|CRITICAL|HIGH|MEDIUM|LOW)\]"
|
||||
r"\*?\*?\s+(?P<rest>.+)$"
|
||||
r"^\s*[-*]\s*[^\w\[]*\[(?P<sev>critical|high|medium|low)\]",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
|
||||
|
||||
@@ -50,10 +59,12 @@ def compress_diff(diff: str, *, context: int = 2) -> tuple[str, int, int]:
|
||||
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.
|
||||
`(text, original_chars, kept_chars)`. `original_chars` is the character
|
||||
length of `diff` as given; `kept_chars` is the character length of
|
||||
`text`. Every emitted hunk header is recomputed to match the lines
|
||||
under it, so the result is a valid unified diff. Lines that are not
|
||||
part of a hunk (`diff --git`, `index …`, `Binary files differ`, mode
|
||||
changes) pass through verbatim.
|
||||
"""
|
||||
if not diff:
|
||||
return diff or "", len(diff or ""), len(diff or "")
|
||||
@@ -64,106 +75,180 @@ def compress_diff(diff: str, *, context: int = 2) -> tuple[str, int, int]:
|
||||
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]
|
||||
m = _HUNK_RE.match(lines[i])
|
||||
if m is None:
|
||||
# File header, index line, binary marker, mode change, prose —
|
||||
# anything outside a hunk body. Copy verbatim.
|
||||
out.append(lines[i])
|
||||
i += 1
|
||||
continue
|
||||
|
||||
# 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]
|
||||
i += 1
|
||||
body_start = i
|
||||
while i < n and _is_body_line(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)
|
||||
out.extend(
|
||||
_render_hunk(
|
||||
body,
|
||||
old_start=int(m.group(1)),
|
||||
new_start=int(m.group(3)),
|
||||
section=m.group(5) or "",
|
||||
context=context,
|
||||
)
|
||||
)
|
||||
|
||||
text = "\n".join(out) + ("\n" if diff.endswith("\n") else "")
|
||||
if not text:
|
||||
# splitlines() dropped nothing-but-newlines; fall back to original.
|
||||
if not text.strip():
|
||||
# Nothing survived (or the input was nothing but newlines); fall back
|
||||
# to the original so the worst case is no improvement, not data loss.
|
||||
return diff, orig, orig
|
||||
if len(text) >= orig:
|
||||
# Re-emitted hunk headers can outweigh the context they replace on a
|
||||
# small, densely-changed diff. Never hand back something longer than
|
||||
# what we were given.
|
||||
return diff, orig, orig
|
||||
return text, orig, len(text)
|
||||
|
||||
|
||||
def _render_hunk_body(body: list[str], *, context: int) -> tuple[list[str], int]:
|
||||
r"""Trim `body` to `context` unchanged lines around the +/- lines.
|
||||
def _is_body_line(line: str) -> bool:
|
||||
r"""True if `line` belongs to the current hunk body.
|
||||
|
||||
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)
|
||||
Hunk bodies contain only ` `/`+`/`-` prefixed lines and `\ No newline at
|
||||
end of file`. An empty line is a context line whose trailing space was
|
||||
stripped (common in mail-formatted diffs), so it counts as body too.
|
||||
|
||||
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.
|
||||
The check is prefix-based *and* header-aware: a removed line reading
|
||||
`---` or an added line reading `+++` (YAML document separators, setext
|
||||
underlines, `--` SQL comments) is body, not a file header — the previous
|
||||
implementation misread those and silently dropped the rest of the hunk.
|
||||
A new file section always opens with `diff --git`, which ends the body.
|
||||
"""
|
||||
if context == 0:
|
||||
# Keep only +/- lines; drop all context.
|
||||
out = [ln for ln in body if ln.startswith("+") or ln.startswith("-")]
|
||||
return out, 0
|
||||
if line == "":
|
||||
return True
|
||||
if line.startswith("diff --git ") or line.startswith("Index: "):
|
||||
return False
|
||||
if _HUNK_RE.match(line):
|
||||
return False
|
||||
return line[0] in " +-\\"
|
||||
|
||||
# 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):
|
||||
def _render_hunk(
|
||||
body: list[str],
|
||||
*,
|
||||
old_start: int,
|
||||
new_start: int,
|
||||
section: str,
|
||||
context: int,
|
||||
) -> list[str]:
|
||||
r"""Trim `body` to `context` unchanged lines around its +/- lines.
|
||||
|
||||
Each surviving run of consecutive lines is emitted as a standalone hunk
|
||||
with a recomputed ``@@ -a,b +c,d @@`` header, so post-change line numbers
|
||||
stay truthful. A hunk with no +/- lines at all (pure context) is dropped
|
||||
entirely; ``\ No newline at end of file`` markers are dropped as noise.
|
||||
|
||||
Returns the rendered lines (headers included), or [] if nothing survived.
|
||||
"""
|
||||
# Number every body line on both sides before anything is dropped.
|
||||
numbered: list[tuple[str, int, int]] = [] # (line, old_no, new_no)
|
||||
old_no, new_no = old_start, new_start
|
||||
for ln in body:
|
||||
if ln.startswith("\\"):
|
||||
continue # `\ No newline at end of file` — no signal, no numbering
|
||||
kind = ln[0] if ln else " "
|
||||
if kind == "+":
|
||||
numbered.append((ln, -1, new_no))
|
||||
new_no += 1
|
||||
elif kind == "-":
|
||||
numbered.append((ln, old_no, -1))
|
||||
old_no += 1
|
||||
else:
|
||||
numbered.append((ln, old_no, new_no))
|
||||
old_no += 1
|
||||
new_no += 1
|
||||
|
||||
changed = [j for j, (ln, _, _) in enumerate(numbered) if ln[:1] in ("+", "-")]
|
||||
if not changed:
|
||||
return []
|
||||
|
||||
keep: set[int] = set()
|
||||
for k in changed:
|
||||
for j in range(max(0, k - context), min(len(numbered) - 1, k + context) + 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
|
||||
for run in _consecutive_runs(sorted(keep)):
|
||||
chunk = [numbered[j] for j in run]
|
||||
old_count = sum(1 for ln, _, _ in chunk if ln[:1] != "+")
|
||||
new_count = sum(1 for ln, _, _ in chunk if ln[:1] != "-")
|
||||
# A run's start is the first line that exists on that side. When a
|
||||
# side has no lines at all (pure addition / pure deletion), unified
|
||||
# diff convention is `start = line before, count = 0`.
|
||||
old_first = next((o for ln, o, _ in chunk if o >= 0), None)
|
||||
new_first = next((nw for ln, _, nw in chunk if nw >= 0), None)
|
||||
old_hdr = old_first if old_first is not None else max(chunk[0][1], 0)
|
||||
new_hdr = new_first if new_first is not None else max(chunk[0][2], 0)
|
||||
if old_count == 0:
|
||||
old_hdr = _side_start_before(numbered, run[0], side=1)
|
||||
if new_count == 0:
|
||||
new_hdr = _side_start_before(numbered, run[0], side=2)
|
||||
out.append(
|
||||
f"@@ -{old_hdr},{old_count} +{new_hdr},{new_count} @@{section}"
|
||||
)
|
||||
out.extend(ln for ln, _, _ in chunk)
|
||||
return out
|
||||
|
||||
return out, len(out)
|
||||
|
||||
def _side_start_before(
|
||||
numbered: list[tuple[str, int, int]], idx: int, *, side: int
|
||||
) -> int:
|
||||
"""Line number on `side` (1=old, 2=new) just before body index `idx`.
|
||||
|
||||
Used for the zero-count header form (`@@ -7,0 +8,3 @@`), where unified
|
||||
diff names the line the change is inserted *after*.
|
||||
"""
|
||||
for j in range(idx - 1, -1, -1):
|
||||
no = numbered[j][side]
|
||||
if no >= 0:
|
||||
return no
|
||||
# Nothing before it: derive from the first numbered line on that side.
|
||||
for _, old_no, new_no in numbered:
|
||||
no = old_no if side == 1 else new_no
|
||||
if no >= 0:
|
||||
return max(no - 1, 0)
|
||||
return 0
|
||||
|
||||
|
||||
def _consecutive_runs(indices: list[int]) -> list[list[int]]:
|
||||
"""Group a sorted index list into runs of consecutive integers."""
|
||||
runs: list[list[int]] = []
|
||||
for j in indices:
|
||||
if runs and j == runs[-1][-1] + 1:
|
||||
runs[-1].append(j)
|
||||
else:
|
||||
runs.append([j])
|
||||
return runs
|
||||
|
||||
|
||||
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.
|
||||
Returns the matching lines stripped of surrounding whitespace, preserving
|
||||
the rendered ``[SEV] `path:line` — problem`` shape (badge emoji and bold
|
||||
markers included, whichever the renderer used). Lines that look like
|
||||
bullets but carry no severity tag are dropped — the reviewer synthesizes
|
||||
from the matched ones. Continuation lines (` - **Fix:** …`) are not
|
||||
finding lines and are dropped with the rest of the prose.
|
||||
"""
|
||||
if not review_body:
|
||||
return []
|
||||
out = []
|
||||
for line in review_body.splitlines():
|
||||
m = _FINDING_BULLET_RE.match(line)
|
||||
if m:
|
||||
if _FINDING_BULLET_RE.match(line):
|
||||
out.append(line.strip())
|
||||
return out
|
||||
return out
|
||||
|
||||
@@ -1084,8 +1084,18 @@ def triage(
|
||||
default_model: str,
|
||||
factory_root: str,
|
||||
) -> list[str] | None:
|
||||
"""Run the triage agent. Returns the lens subset with surface, or None to
|
||||
mean "all reviewers" (fail-open on any error).
|
||||
"""Run the triage agent. Returns the lens subset with surface.
|
||||
|
||||
Three outcomes, kept distinct on purpose:
|
||||
|
||||
* ``[lens, …]`` — run exactly these.
|
||||
* ``[]`` — the agent deliberately returned an empty list: no lens
|
||||
has surface on this diff, so the fan-out is skipped entirely. Only a
|
||||
literally-empty ``lenses`` list produces this.
|
||||
* ``None`` — fail open, run everything. Covers triage disabled, a
|
||||
crash, unparseable output, a malformed `lenses` value, AND the case
|
||||
where the agent named only ids that don't exist (a hallucinated roster
|
||||
is not a verdict of "nothing to review").
|
||||
|
||||
`triage_cfg.enabled = False` → skip triage, return None.
|
||||
"""
|
||||
@@ -1125,7 +1135,20 @@ def triage(
|
||||
lenses = obj.get("lenses")
|
||||
if not isinstance(lenses, list):
|
||||
return None
|
||||
if not lenses:
|
||||
# Deliberate "no lens needed" verdict — the one case that skips.
|
||||
print("pragent: triage selected no lenses (no review surface)", flush=True)
|
||||
return []
|
||||
valid = [lid for lid in lenses if isinstance(lid, str) and lid in lens_ids]
|
||||
if not valid:
|
||||
# The agent named lenses, but none of them exist. That's a bad roster,
|
||||
# not an empty one — fail open rather than silently skipping the review.
|
||||
print(
|
||||
f"pragent: triage named no known lenses ({lenses!r}); "
|
||||
f"falling back to all lenses",
|
||||
flush=True,
|
||||
)
|
||||
return None
|
||||
cap = triage_cfg.get("max_lenses", 5)
|
||||
selected = valid[:cap]
|
||||
print(f"pragent: triage selected {selected}", flush=True)
|
||||
@@ -1133,13 +1156,19 @@ def triage(
|
||||
|
||||
|
||||
def _intersect_with_triage(
|
||||
reviewers: list[ReviewerSpec], selected_ids: list[str]
|
||||
reviewers: list[ReviewerSpec], selected_ids: list[str] | None
|
||||
) -> list[ReviewerSpec]:
|
||||
"""Filter `reviewers` to those named by `selected_ids`, preserving the
|
||||
original order. Lenses in `selected_ids` not present in `reviewers` are
|
||||
dropped silently. `None` or empty list → no triage, return all."""
|
||||
if not selected_ids:
|
||||
return list(reviewers)
|
||||
dropped silently.
|
||||
|
||||
An empty `selected_ids` yields an empty result — "triage picked nothing"
|
||||
is a real verdict and the caller short-circuits on it. Fail-open is
|
||||
signalled by `triage()` returning None, never by an empty list; conflating
|
||||
the two made a "no review surface" verdict run every lens instead.
|
||||
"""
|
||||
if selected_ids is None:
|
||||
return list(reviewers) # fail-open: triage produced no verdict
|
||||
sel = set(selected_ids)
|
||||
return [r for r in reviewers if r.id in sel]
|
||||
|
||||
@@ -1234,10 +1263,18 @@ def run_lenses_review(
|
||||
workdir, triage_cfg, reviewers, model, _factory_dir(),
|
||||
)
|
||||
if selected is not None:
|
||||
reviewers = _intersect_with_triage(reviewers, selected) or reviewers
|
||||
if not selected:
|
||||
# Triage says nothing here has review surface. Skip the
|
||||
# fan-out and post a clean empty review — running all N
|
||||
# lenses anyway would burn N subprocesses to contradict it.
|
||||
return _no_surface_response(repo, index, sha, len(reviewers))
|
||||
reviewers = _intersect_with_triage(reviewers, selected)
|
||||
|
||||
if not reviewers:
|
||||
return "", None
|
||||
# Every lens was filtered out (skip_if_all_changed_paths, or a
|
||||
# triage subset naming lenses this repo doesn't enable). Same
|
||||
# outcome as the triage skip: nothing to run, nothing to say.
|
||||
return _no_surface_response(repo, index, sha, 0)
|
||||
|
||||
factory_root = _factory_dir()
|
||||
results = run_lenses(workdir, reviewers, model, factory_root)
|
||||
@@ -1282,6 +1319,37 @@ def run_lenses_review(
|
||||
shutil.rmtree(workdir, ignore_errors=True)
|
||||
|
||||
|
||||
def _no_surface_response(
|
||||
repo: str, index: str, sha: str, n_lenses: int
|
||||
) -> tuple[str, dict | None]:
|
||||
"""A well-formed 'nothing to review' result for the no-lens paths.
|
||||
|
||||
Returns the same shape every other path returns — prose plus a final
|
||||
```json fence with an empty `findings` array — so
|
||||
`ai_review.parse_review_output` parses it normally. Returning bare `""`
|
||||
here (the old behaviour) landed in ai_review's unparseable-output branch
|
||||
and posted "AI review produced no parseable output", which reads as a
|
||||
malfunction rather than a verdict.
|
||||
"""
|
||||
if n_lenses:
|
||||
summary = (
|
||||
f"Triage found no review surface in {repo}#{index} "
|
||||
f"(sha {sha[:8]}): none of the {n_lenses} configured lens(es) "
|
||||
f"apply to this diff. No findings."
|
||||
)
|
||||
else:
|
||||
summary = (
|
||||
f"No lens applies to {repo}#{index} (sha {sha[:8]}) after path "
|
||||
f"filtering. No findings."
|
||||
)
|
||||
text = (
|
||||
f"{summary}\n\n"
|
||||
f"## Findings (multi-lens)\n\n"
|
||||
f"```json\n{json.dumps({'summary': summary, 'findings': []}, indent=2)}\n```\n"
|
||||
)
|
||||
return text, None
|
||||
|
||||
|
||||
def _fallback_single_primary(workdir: str, model: str) -> tuple[str, dict | None]:
|
||||
"""Used when reviewers[] resolves to empty (all activation:off)."""
|
||||
try:
|
||||
|
||||
Reference in New Issue
Block a user