feat(review): equivalent provider price + enriched .pr-review.json schema

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          <PRICES key>              (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.
This commit is contained in:
Marcos
2026-08-20 16:12:46 +00:00
parent 770581bf53
commit ec26ec000a
3 changed files with 668 additions and 32 deletions
+415 -29
View File
@@ -67,8 +67,27 @@ _SHA_MARKER_RE = re.compile(r"<!-- pragent:sha=([0-9a-f]{7,40}) -->")
AI_REVIEW_LABEL = "AI-REVIEW" AI_REVIEW_LABEL = "AI-REVIEW"
SEVERITIES = ("critical", "high", "medium", "low") 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" 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. 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 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 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. """Render the `## 🔋 AI usage` block for the review body.
Only called when the PR carries the `AI-USAGE` label (and the opencode 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 (input/output/reasoning/cache/cost/steps/duration) plus an ATTRIBUTED
per-finding table — one model pass generates all findings, so per-comment per-finding table — one model pass generates all findings, so per-comment
counts are an estimate (output split by body weight), clearly labelled. 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. Returns "" if `usage` is None.
""" """
if not usage: if not usage:
return "" return ""
dur = usage.get("duration_s") dur = usage.get("duration_s")
dur_s = f"{dur}s" if dur is not None else "?" dur_s = f"{dur}s" if dur is not None else "?"
cost = usage.get("cost") or 0.0 actual = usage.get("cost") or 0.0
cost_s = f"${cost:.4f}" if cost else "$0.00" actual_s = f"${actual:.4f}" if actual else "$0.00"
cost_note = ( actual_note = (
"(on-network glm-5.2:cloud via headroom — no per-token charge)" "(headroom glm-5.2:cloud — free tier)"
if not cost else "(billed by provider)" 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 = [ lines = [
"## 🔋 AI usage", "## 🔋 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('cache_read', 0)} read / {usage.get('cache_write', 0)} write "
f"{usage.get('total', 0)} total" 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", "- 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-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.""" """Assemble the user prompt: repo config + prior reviews + PR meta + diff."""
parts: list[str] = [] parts: list[str] = []
if config: eff = effective_config(config) if config else {}
if eff:
cfg_lines = [] cfg_lines = []
if config.get("focus"): if eff.get("focus"):
cfg_lines.append("Focus areas: " + ", ".join(config["focus"])) cfg_lines.append("Focus areas: " + ", ".join(eff["focus"]))
if config.get("exclude_paths"): if eff.get("exclude_paths"):
cfg_lines.append("Ignore paths: " + ", ".join(config["exclude_paths"])) cfg_lines.append("Ignore paths: " + ", ".join(eff["exclude_paths"]))
if config.get("languages"): if eff.get("languages"):
cfg_lines.append("Languages: " + ", ".join(config["languages"])) cfg_lines.append("Languages: " + ", ".join(eff["languages"]))
if config.get("instructions"): if eff.get("style"):
cfg_lines.append("Instructions:\n" + str(config["instructions"]).strip()) 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: if cfg_lines:
parts.append("## Repo review config (.pr-review.json)\n" + "\n".join(cfg_lines)) parts.append("## Repo review config (.pr-review.json)\n" + "\n".join(cfg_lines))
if prior_reviews: if prior_reviews:
joined = "\n\n---\n\n".join(prior_reviews) joined = "\n\n---\n\n".join(prior_reviews)
if len(joined) > 8000: if len(joined) > 4000:
joined = joined[:8000] + "\n…[prior reviews truncated]" joined = joined[:4000] + "\n…[prior reviews truncated]"
parts.append("## PREVIOUS REVIEWS (already posted — do NOT repeat these points)\n" + joined) parts.append("## PREVIOUS REVIEWS (already posted — do NOT repeat these points)\n" + joined)
parts.append(f"## PR\nTitle: {title or '(none)'}") 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_LIST_ITEMS = 32
CONFIG_MAX_ITEM_CHARS = 200 CONFIG_MAX_ITEM_CHARS = 200
CONFIG_MAX_INSTRUCTIONS_CHARS = 4000 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: def parse_repo_config(raw: str) -> dict:
"""Parse a .pr-review.json blob tolerantly. Returns {} on any failure. """Parse a .pr-review.json blob tolerantly. Returns {} on any failure.
List fields are capped at CONFIG_MAX_LIST_ITEMS entries of 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 <key of cost_model.PRICES> — see equivalent_cost
""" """
if not raw: if not raw:
return {} return {}
@@ -654,17 +792,195 @@ def parse_repo_config(raw: str) -> dict:
return {} return {}
if not isinstance(data, dict): if not isinstance(data, dict):
return {} return {}
out = {}
for k in ("focus", "exclude_paths", "languages"): def _str_list(v):
v = data.get(k)
if isinstance(v, list) and all(isinstance(x, str) for x in 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") instr = data.get("instructions")
if isinstance(instr, str) and instr.strip(): if isinstance(instr, str) and instr.strip():
out["instructions"] = instr.strip()[:CONFIG_MAX_INSTRUCTIONS_CHARS] 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 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]: def reviewed_shas(reviews: list[dict]) -> set[str]:
"""Pull every `<!-- pragent:sha=... -->` marker out of a PR's reviews.""" """Pull every `<!-- pragent:sha=... -->` marker out of a PR's reviews."""
shas: set[str] = set() shas: set[str] = set()
@@ -693,6 +1009,29 @@ def prior_review_bodies(reviews: list[dict], current_sha: str, limit: int = 6) -
return out[:limit] 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 # Network helpers
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
@@ -911,13 +1250,35 @@ def review_pr(
print(f"pragent: {repo}#{index} sha={sha[:8]} already reviewed, skipping", flush=True) print(f"pragent: {repo}#{index} sha={sha[:8]} already reviewed, skipping", flush=True)
return True return True
diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars) raw_diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars)
if not diff.strip(): if not raw_diff.strip():
post_review(api, repo, index, token, format_review_body("No diff content to review.", model, sha)) post_review(api, repo, index, token, format_review_body("No diff content to review.", model, sha))
return True return True
config = fetch_repo_config(api, repo, token, ref=base_ref) 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() engine = os.environ.get("PRAGENT_ENGINE", "opencode").strip().lower()
review_summary = "" review_summary = ""
@@ -934,6 +1295,7 @@ def review_pr(
api=api, repo=repo, index=index, sha=sha, token=token, api=api, repo=repo, index=index, sha=sha, token=token,
title=title, body=body, diff=diff, config=config, title=title, body=body, diff=diff, config=config,
prior_reviews=prior, model=oc_model, prior_reviews=prior, model=oc_model,
compression_note=compression_note,
) )
review_summary, findings = parse_review_output(stdout) review_summary, findings = parse_review_output(stdout)
if not findings and not review_summary: if not findings and not review_summary:
@@ -949,24 +1311,48 @@ def review_pr(
salvaged = salvage_summary(stdout) salvaged = salvage_summary(stdout)
usage_section = "" usage_section = ""
if report_usage and usage: 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( post_review(api, repo, index, token, format_review_body(
salvaged or "AI review produced no parseable output.", salvaged or "AI review produced no parseable output.",
model, sha, usage_section=usage_section)) model, sha, usage_section=usage_section))
return True return True
else: 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) raw_findings = call_model(ollama_url, model, SYSTEM_PROMPT, user_prompt, max_tokens)
findings = parse_findings(raw_findings) findings = parse_findings(raw_findings)
usage = None 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 # Attribute output tokens to each finding (mutates finding dicts) so
# inline comments + the usage table can show a per-comment estimate. # inline comments + the usage table can show a per-comment estimate.
# Only meaningful when we have measured usage AND the PR asked for it. # Only meaningful when we have measured usage AND the PR asked for it.
usage_section = "" usage_section = ""
if report_usage and usage and usage.get("output"): if report_usage and usage and usage.get("output"):
compute_attribution(findings, usage["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) anchors = parse_diff_anchors(diff)
anchored, unanchored = split_findings(findings, anchors) anchored, unanchored = split_findings(findings, anchors)
+12 -3
View File
@@ -286,6 +286,7 @@ def write_brief(
diff: str, diff: str,
config: dict | None, config: dict | None,
prior_reviews: list[str] | None, prior_reviews: list[str] | None,
compression_note: str = "",
) -> str: ) -> str:
"""Render `.pragent/brief.md` in the workdir. Returns the path written.""" """Render `.pragent/brief.md` in the workdir. Returns the path written."""
path = os.path.join(workdir, ".pragent") path = os.path.join(workdir, ".pragent")
@@ -297,16 +298,17 @@ def write_brief(
prior = "_(none)_" prior = "_(none)_"
if prior_reviews: if prior_reviews:
prior = "\n\n---\n\n".join(prior_reviews) prior = "\n\n---\n\n".join(prior_reviews)
if len(prior) > 8000: if len(prior) > 4000:
prior = prior[:8000] + "\n…[prior reviews truncated]" prior = prior[:4000] + "\n…[prior reviews truncated]"
files = changed_files(diff) files = changed_files(diff)
files_block = "\n".join(f"- `{p}`" for p in files) if files else "_(none)_" 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( content = _BRIEF_TEMPLATE.format(
repo=repo or "?", repo=repo or "?",
index=index or "?", index=index or "?",
sha=sha or "?", sha=sha or "?",
title=title or "(none)", title=title or "(none)",
description=description.strip() or "_(none)_", description=desc_block,
changed_files=files_block, changed_files=files_block,
config=cfg, config=cfg,
prior=prior, prior=prior,
@@ -672,6 +674,7 @@ def run(
config: dict | None, config: dict | None,
prior_reviews: list[str] | None, prior_reviews: list[str] | None,
model: str, model: str,
compression_note: str = "",
) -> tuple[str, dict | None]: ) -> tuple[str, dict | None]:
"""End-to-end: checkout archive → brief → drop factory → opencode → (text, usage). """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 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`) 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. 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) os.makedirs(WORK_ROOT, exist_ok=True)
workdir = tempfile.mkdtemp(prefix=f"{repo.replace('/', '_')}-{sha[:8]}-", dir=WORK_ROOT) workdir = tempfile.mkdtemp(prefix=f"{repo.replace('/', '_')}-{sha[:8]}-", dir=WORK_ROOT)
@@ -697,6 +705,7 @@ def run(
workdir, workdir,
repo=repo, index=index, sha=sha, title=title, description=body, repo=repo, index=index, sha=sha, title=title, description=body,
diff=diff, config=config, prior_reviews=prior_reviews, diff=diff, config=config, prior_reviews=prior_reviews,
compression_note=compression_note,
) )
drop_factory(workdir) drop_factory(workdir)
text, usage = run_opencode(workdir, model) text, usage = run_opencode(workdir, model)
+241
View File
@@ -779,3 +779,244 @@ def test_salvage_summary_empty_when_nothing_to_salvage():
assert ai_review.salvage_summary("") == "" assert ai_review.salvage_summary("") == ""
assert ai_review.salvage_summary(" \n ") == "" assert ai_review.salvage_summary(" \n ") == ""
assert ai_review.salvage_summary("```json\n{}\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<!-- pragent:sha=abc -->",
"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) == []