diff --git a/.opencode/agents/triage.md b/.opencode/agents/triage.md index 29720b7..1aece1a 100644 --- a/.opencode/agents/triage.md +++ b/.opencode/agents/triage.md @@ -46,5 +46,8 @@ Output STRICT JSON, nothing else, on a single line: ``` If `reviewers[]` is empty or absent, output `{"lenses":[]}`. The caller -treats `[]` as "no lenses needed" and skips the fan-out. Never refuse, +treats `[]` as "no lenses needed" and skips the fan-out โ€” an empty list is +the only way to skip, so use it deliberately. Only ever name ids from the +roster you were given: a list containing no known id is treated as a bad +answer and the caller falls back to running every lens. Never refuse, never explain, never add prose. \ No newline at end of file diff --git a/pilot/ai_review.py b/pilot/ai_review.py index d64d1a7..fec53ea 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -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 ` 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 diff --git a/pilot/diff_compress.py b/pilot/diff_compress.py index 24d3ea9..f50fc88 100644 --- a/pilot/diff_compress.py +++ b/pilot/diff_compress.py @@ -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 `- [,]` 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. +# 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*\*?\*?\[(?Pcritical|high|medium|low|CRITICAL|HIGH|MEDIUM|LOW)\]" - r"\*?\*?\s+(?P.+)$" + r"^\s*[-*]\s*[^\w\[]*\[(?Pcritical|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 \ No newline at end of file + return out diff --git a/pilot/opencode_review.py b/pilot/opencode_review.py index 7c67f12..299eb5d 100644 --- a/pilot/opencode_review.py +++ b/pilot/opencode_review.py @@ -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: diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index cf11087..2ab4d08 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -19,7 +19,6 @@ from ai_review import ( # noqa: E402 compute_attribution, findings_table, format_review_body, - format_usage_section, inline_comment_body, parse_diff_anchors, parse_findings, @@ -604,6 +603,31 @@ def test_inline_comment_body_renders_reference(): assert "๐Ÿ”— **Reference:** [cve.example/X](https://cve.example/X)" in body +def test_reference_non_url_renders_as_plain_text(): + # A CVE id or doc title is not a URL. `[CVE-2024-1](CVE-2024-1)` renders as + # a broken *relative* link in Gitea, so bare text is the correct fallback. + assert ai_review._format_reference("CVE-2024-1234") == "CVE-2024-1234" + assert ai_review._format_reference("see OWASP A03") == "see OWASP A03" + assert ai_review._format_reference("") == "" + f = {"severity": "high", "path": "a", "line": 1, "problem": "p", "fix": "", + "suggestion": "", "reference": "CVE-2024-1234"} + body = inline_comment_body(f) + assert "๐Ÿ”— **Reference:** CVE-2024-1234" in body + assert "](CVE-" not in body + + +def test_int_env_falls_back_on_garbage(monkeypatch, capsys): + monkeypatch.setenv("PRAGENT_DIFF_CONTEXT", "two") + assert ai_review._int_env("PRAGENT_DIFF_CONTEXT", 1) == 1 + assert "ignoring PRAGENT_DIFF_CONTEXT" in capsys.readouterr().err + monkeypatch.setenv("PRAGENT_DIFF_CONTEXT", " 3 ") + assert ai_review._int_env("PRAGENT_DIFF_CONTEXT", 1) == 3 + monkeypatch.setenv("PRAGENT_DIFF_CONTEXT", "") + assert ai_review._int_env("PRAGENT_DIFF_CONTEXT", 1) == 1 + monkeypatch.delenv("PRAGENT_DIFF_CONTEXT") + assert ai_review._int_env("PRAGENT_DIFF_CONTEXT", -1) == -1 + + def test_inline_comment_body_no_reference_no_ref_line(): f = {"severity": "low", "path": "a", "line": 1, "problem": "p", "fix": "", "suggestion": "", "reference": ""} @@ -641,7 +665,7 @@ def test_format_review_body_with_summary_section(): # --------------------------------------------------------------------------- -# AI-USAGE: compute_attribution + format_usage_section + inline ๐Ÿช™ line +# AI-USAGE: compute_attribution + usage block + inline ๐Ÿช™ line # --------------------------------------------------------------------------- @@ -695,47 +719,30 @@ def test_inline_comment_body_no_attribution_no_coin_line(): assert "๐Ÿช™" not in inline_comment_body(f) -def test_format_usage_section_renders_totals_and_table(): - fs = [ - {"severity": "critical", "path": "src/Foo.java", "line": 98, - "problem": "p"*10, "fix": "f", "suggestion": "", "_tok_attrib": 180, "_tok_pct": 0.29}, - ] +def test_render_collapsible_usage_renders_totals(): usage = {"input": 18420, "output": 612, "reasoning": 0, "cache_read": 15210, "cache_write": 0, "total": 19032, "cost": 0.0, "steps": 7, "duration_s": 142.0} - sec = format_usage_section(usage, fs, "glm-5.2:cloud") - assert "## ๐Ÿ”‹ AI usage" in sec + sec = _render_collapsible_usage(usage, "glm-5.2:cloud", config=None) + assert "๐Ÿ”‹ AI Usage & Run Details" in sec assert "`glm-5.2:cloud`" in sec - assert "agent steps: 7" in sec - assert "duration: 142.0s" in sec - assert "18420 in" in sec and "612 out" in sec and "19032 total" in sec + assert "7 steps" in sec + assert "142.0s" in sec + assert "18420 in / 612 out" in sec and "19032 total" in sec assert "$0.00" in sec - assert "whole-repo checkout" in sec + assert "Whole-repo checkout" in sec assert "attributed" in sec - # table - assert "| severity | location | โ‰ˆout tok | % |" in sec - assert "CRITICAL" in sec - assert "`src/Foo.java:98`" in sec - assert "180" in sec and "29%" in sec -def test_format_usage_section_omits_table_when_no_attributed_rows(): - usage = {"input": 10, "output": 0, "reasoning": 0, "cache_read": 0, - "cache_write": 0, "total": 10, "cost": 0.0, "steps": 1, "duration_s": 1.0} - sec = format_usage_section(usage, [], "glm-5.2:cloud") - assert "## ๐Ÿ”‹ AI usage" in sec - assert "severity | location" not in sec # no rows โ†’ no table +def test_render_collapsible_usage_none_returns_empty(): + assert _render_collapsible_usage(None, "m", config=None) == "" -def test_format_usage_section_none_returns_empty(): - assert format_usage_section(None, [], "m") == "" - - -def test_format_usage_section_cost_nonzero(): +def test_render_collapsible_usage_cost_nonzero_drops_free_tier_note(): usage = {"input": 10, "output": 0, "reasoning": 0, "cache_read": 0, "cache_write": 0, "total": 10, "cost": 0.0123, "steps": 1, "duration_s": 1.0} - sec = format_usage_section(usage, [], "m") + sec = _render_collapsible_usage(usage, "m", config=None) assert "$0.0123" in sec - assert "billed by provider" in sec + assert "free tier" not in sec def test_format_review_body_usage_section_below_findings(): @@ -1022,7 +1029,7 @@ def test_salvage_summary_empty_when_nothing_to_salvage(): # --------------------------------------------------------------------------- -# equivalent_cost + format_usage_section equivalent-provider line +# equivalent_cost + usage-block equivalent-provider line # --------------------------------------------------------------------------- @@ -1037,15 +1044,15 @@ 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(): +def test_usage_block_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") + sec = ai_review._render_collapsible_usage(usage, "glm-5.2:cloud", config=None) # 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 "๐Ÿ”‹ AI Usage & Run Details" 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 @@ -1057,36 +1064,36 @@ def test_format_usage_section_shows_equivalent_provider_cost(): assert "$0.00" not in cost_lines[0] -def test_format_usage_section_honors_cost_target(monkeypatch): +def test_usage_block_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") + sec = ai_review._render_collapsible_usage(usage, "glm-5.2:cloud", config=None) 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): +def test_usage_block_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"} + sec = ai_review._render_collapsible_usage( + 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(): +def test_usage_block_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"} + sec = ai_review._render_collapsible_usage( + 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 diff --git a/tests/pilot/test_diff_compress.py b/tests/pilot/test_diff_compress.py index 7af959a..4925241 100644 --- a/tests/pilot/test_diff_compress.py +++ b/tests/pilot/test_diff_compress.py @@ -1,5 +1,6 @@ """Unit tests for pragent pilot diff_compress. No network.""" import os +import re import sys HERE = os.path.dirname(os.path.abspath(__file__)) @@ -20,7 +21,7 @@ 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 @@ +@@ -1,20 +1,21 @@ ctx1 -removed +added @@ -30,8 +31,17 @@ index 1..2 100644 ctx5 ctx6 ctx7 -+extra ctx8 + ctx9 + ctx10 + ctx11 + ctx12 + ctx13 + ctx14 + ctx15 + ctx16 ++extra + ctx17 @@ -20,3 +21,4 @@ tail1 tail2 @@ -76,11 +86,12 @@ def test_compress_diff_negative_disables_compression(): 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. +def test_compress_diff_collapsed_gap_splits_into_two_hunks(): + # Two +/- lines separated by 14 context lines, context=2. The dropped + # middle is expressed by SPLITTING the hunk in two, each with a recomputed + # `@@` header โ€” not by a pseudo-marker line. `parse_diff_anchors` reads + # `@@` headers to reset its line counter, so anything that looks like a + # header but isn't one silently misanchors every following comment. middle = "\n".join(f" m{i}" for i in range(14)) + "\n" # trailing \n! diff = ( "diff --git a/x.py b/x.py\n" @@ -95,7 +106,12 @@ def test_compress_diff_collapsed_gap_marker(): ) text, _, _ = compress_diff(diff, context=2) assert "+a" in text and "+b" in text - assert "@@ โ€ฆ" in text and "context line(s) omitted" in text + for m in ("m2", "m3", "m4", "m5", "m6", "m7", "m8", "m9", "m10", "m11"): + assert f" {m}\n" not in text # the gap itself is gone + # Two hunks, and every emitted header is a real unified-diff header. + headers = [ln for ln in text.splitlines() if ln.startswith("@@")] + assert len(headers) == 2 + assert all(re.match(r"^@@ -\d+,\d+ \+\d+,\d+ @@", h) for h in headers) def test_compress_diff_strips_no_newline_marker(): @@ -245,4 +261,78 @@ def test_compress_diff_preserves_anchors_for_post_change_lines(): 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 + assert 11 in anchors["x.py"] + +def test_compress_diff_keeps_post_change_line_numbers_exact(): + # The regression that motivated the hunk-header rewrite: dropping context + # lines without renumbering shifted every anchor. Here `+new` really is + # line 10 of the post-change file; compression must not move it. + raw = ( + "diff --git a/x.py b/x.py\n" + "--- a/x.py\n" + "+++ b/x.py\n" + "@@ -1,12 +1,12 @@\n" + + "".join(f" l{i}\n" for i in range(1, 10)) + + "-old\n" + + "+new\n" + + " l11\n" + ) + import ai_review + raw_anchors = ai_review.parse_diff_anchors(raw)["x.py"] + assert 10 in raw_anchors # +new + text, _, _ = compress_diff(raw, context=1) + comp_anchors = ai_review.parse_diff_anchors(text)["x.py"] + # Compression only ever drops anchors; it never invents or moves one. + assert comp_anchors <= raw_anchors + assert 10 in comp_anchors # +new still anchors to its real line + + +def test_compress_diff_content_line_starting_with_dashes_is_not_a_header(): + # A removed YAML document separator renders as `----`; an added one as + # `+++new`. Treating those as file headers truncated the hunk body and + # dropped the `@@` header with it. + diff = ( + "diff --git a/x.yml b/x.yml\n" + "--- a/x.yml\n" + "+++ b/x.yml\n" + "@@ -1,4 +1,4 @@\n" + " a: 1\n" + " b: 2\n" + "----\n" + "+++new\n" + " c: 3\n" + ) + text, _, _ = compress_diff(diff, context=1) + assert "----" in text and "+++new" in text + # The hunk header survives, so the body is still anchorable. + headers = [ln for ln in text.splitlines() if _is_hunk_header(ln)] + assert len(headers) == 1 + import ai_review + assert ai_review.parse_diff_anchors(text)["x.yml"] == {2, 3, 4} + + +def _is_hunk_header(line: str) -> bool: + return bool(re.match(r"^@@ -\d+,\d+ \+\d+,\d+ @@", line)) + + +def test_extract_finding_bullets_matches_current_renderer_output(): + # The prior-review dedupe is only worth anything if it can read the + # bullets pragent itself posts. `summary_bullets` renders an emoji badge + # between the `-` and the `[SEV]` tag, which the original regex rejected. + import ai_review + findings = [ + {"path": "a.py", "line": 10, "severity": "high", + "problem": "boom", "fix": "guard it", "suggestion": "", "reference": ""}, + {"path": "b.go", "line": 0, "severity": "low", + "problem": "nit", "fix": "", "suggestion": "", "reference": ""}, + ] + body = ai_review.format_review_body( + ai_review.summary_bullets(findings), "m", "abc123", + findings_for_table=findings, + ) + bullets = extract_finding_bullets(body) + assert len(bullets) == 2 + assert any("a.py:10" in b and "boom" in b for b in bullets) + # `**Fix:**` continuation lines are prose, not findings. + assert all("**Fix:**" not in b for b in bullets) + assert ai_review.compact_prior_reviews([body]) != [] diff --git a/tests/pilot/test_opencode_review.py b/tests/pilot/test_opencode_review.py index e1529b1..1209b24 100644 --- a/tests/pilot/test_opencode_review.py +++ b/tests/pilot/test_opencode_review.py @@ -750,10 +750,14 @@ def test_intersect_with_triage_preserves_order(): assert [r.id for r in out] == ["security", "docs"] -def test_intersect_with_triage_none_returns_all(): +def test_intersect_with_triage_none_fails_open_but_empty_selects_nothing(): + # The two must NOT be conflated: None is "triage gave no verdict, run + # everything"; [] is "triage says no lens has surface", which the caller + # short-circuits on. Returning all lenses for [] made a skip verdict run + # every lens instead. reviewers = [oc.ReviewerSpec(id="security"), oc.ReviewerSpec(id="docs")] assert oc._intersect_with_triage(reviewers, None) == reviewers - assert oc._intersect_with_triage(reviewers, []) == reviewers + assert oc._intersect_with_triage(reviewers, []) == [] def test_merge_usage_sums_tokens(): @@ -772,3 +776,84 @@ def test_merge_usage_skips_none(): merged = oc.merge_usage([a, None, None]) assert merged["input"] == 100 assert merged["steps"] == 3 + + +# --------------------------------------------------------------------------- +# triage(): the empty-list verdict must survive as its own outcome +# --------------------------------------------------------------------------- + + +def _stub_triage_env(monkeypatch, agent_output: str): + """Make `triage()` runnable in-process: no opencode binary, no HOME setup.""" + class _Proc: + stdout = "irrelevant โ€” parse_opencode_events is stubbed" + stderr = "" + returncode = 0 + + monkeypatch.setattr(oc, "_opencode_bin", lambda: "/bin/true") + monkeypatch.setattr(oc, "_shared_home", lambda: "/tmp") + monkeypatch.setattr(oc, "_warm_opencode", lambda home, model: None) + monkeypatch.setattr(oc, "_build_env", lambda home: {}) + monkeypatch.setattr(oc.subprocess, "run", lambda *a, **k: _Proc()) + monkeypatch.setattr(oc, "parse_opencode_events", lambda raw: (agent_output, None)) + + +_TRIAGE_CFG = {"enabled": True, "model": "", "max_lenses": 5} + + +def test_triage_empty_list_is_a_skip_verdict(monkeypatch): + _stub_triage_env(monkeypatch, '{"lenses":[]}') + reviewers = [oc.ReviewerSpec(id="security"), oc.ReviewerSpec(id="docs")] + out = oc.triage("/tmp", _TRIAGE_CFG, reviewers, "m", "/tmp") + # [] โ€” NOT None. None would fail open and run every lens. + assert out == [] + assert out is not None + + +def test_triage_unknown_lens_ids_fail_open(monkeypatch): + # A hallucinated roster is a bad answer, not a verdict of "nothing to + # review" โ€” it must fail open rather than silence the whole review. + _stub_triage_env(monkeypatch, '{"lenses":["not-a-lens","also-fake"]}') + reviewers = [oc.ReviewerSpec(id="security"), oc.ReviewerSpec(id="docs")] + assert oc.triage("/tmp", _TRIAGE_CFG, reviewers, "m", "/tmp") is None + + +def test_triage_valid_subset_selected(monkeypatch): + _stub_triage_env(monkeypatch, '{"lenses":["docs","nope"]}') + reviewers = [oc.ReviewerSpec(id="security"), oc.ReviewerSpec(id="docs")] + assert oc.triage("/tmp", _TRIAGE_CFG, reviewers, "m", "/tmp") == ["docs"] + + +def test_triage_disabled_fails_open(monkeypatch): + _stub_triage_env(monkeypatch, '{"lenses":[]}') + reviewers = [oc.ReviewerSpec(id="security")] + cfg = {"enabled": False, "model": "", "max_lenses": 5} + assert oc.triage("/tmp", cfg, reviewers, "m", "/tmp") is None + + +def test_triage_malformed_output_fails_open(monkeypatch): + _stub_triage_env(monkeypatch, "the agent wrote prose instead of JSON") + reviewers = [oc.ReviewerSpec(id="security")] + assert oc.triage("/tmp", _TRIAGE_CFG, reviewers, "m", "/tmp") is None + + +def test_no_surface_response_parses_as_an_empty_review(): + # The skip path must return the same shape every other path returns. + # A bare "" landed in ai_review's unparseable-output branch and posted + # "AI review produced no parseable output" โ€” a malfunction, not a verdict. + import ai_review + text, usage = oc._no_surface_response("o/r", "9", "abc12345", 3) + assert usage is None + summary, findings, _changes, _risks = ai_review.parse_review_output(text) + assert findings == [] + assert summary # non-empty, so ai_review does NOT take the salvage branch + assert "no review surface" in summary.lower() + assert "3 configured lens" in summary + + +def test_no_surface_response_zero_lenses_wording(): + import ai_review + text, _ = oc._no_surface_response("o/r", "9", "abc12345", 0) + summary, findings, _c, _r = ai_review.parse_review_output(text) + assert findings == [] + assert "after path filtering" in summary