diff --git a/pilot/ai_review.py b/pilot/ai_review.py index ca5620d..a10bb18 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -67,8 +67,27 @@ _SHA_MARKER_RE = re.compile(r"") AI_REVIEW_LABEL = "AI-REVIEW" SEVERITIES = ("critical", "high", "medium", "low") +# Severity rank โ€” higher = more severe. Used by `apply_repo_config` to drop +# findings below `severity_threshold`. Critical=3, high=2, medium=1, low=0. +SEVERITY_RANK = {"low": 0, "medium": 1, "high": 2, "critical": 3} REPO_CONFIG_FILE = ".pr-review.json" +# Style โ†’ (default max_findings, default severity_threshold). Strict is +# terse/high-signal; lenient shows everything; balanced is the default for +# unconfigured repos. Repo `.pr-review.json` overrides per-field. +STYLE_DEFAULTS: dict[str, tuple[int, str]] = { + "strict": (5, "high"), + "balanced": (12, "medium"), + "lenient": (15, "low"), +} + +# Default provider to compare against in the usage section. The pilot runs on +# headroom/glm-5.2:cloud at $0/MTok, so the actual line shows $0.00 โ€” but the +# equivalent provider line lets a maintainer see what they would have paid on +# Claude/GPT for the same measured tokens. Override with PRAGENT_PRICE_TARGET +# (env) or `.pr-review.json:cost_target` (per repo). +DEFAULT_PRICE_TARGET = "claude-sonnet-5" + SYSTEM_PROMPT = """You are a senior, pragmatic code reviewer. Review the pull request diff below. Report ONLY real, actionable issues: correctness bugs, security problems, risky @@ -197,7 +216,76 @@ def compute_attribution(findings: list[dict], output_tokens: int) -> None: f["_tok_pct"] = w / total_w -def format_usage_section(usage: dict | None, findings: list[dict], model: str) -> str: +def _resolve_price_target(config: dict | None) -> tuple[str, str | None]: + """Pick which provider to compute the equivalent cost against. + + Order: `.pr-review.json:cost_target` > `PRAGENT_PRICE_TARGET` env > + `DEFAULT_PRICE_TARGET` (claude-sonnet-5). Returns `(price_key, error)`. + + If any of the user-set keys is unknown, falls back to the default AND + reports the error so the operator sees their typo (a config-level typo + silently picking the default would defeat the purpose of letting repos + opt into a different comparison model). + """ + from cost_model import PRICES # local import keeps ollama path dep-free + candidates: list[tuple[str, str]] = [] + if isinstance(config, dict) and config.get("cost_target"): + candidates.append(("repo config", str(config["cost_target"]).strip())) + env = os.environ.get("PRAGENT_PRICE_TARGET", "").strip() + if env: + candidates.append(("PRAGENT_PRICE_TARGET env", env)) + candidates.append(("default", DEFAULT_PRICE_TARGET)) + + chosen = DEFAULT_PRICE_TARGET + for source, key in candidates: + if key in PRICES: + chosen = key + break + else: + # No candidate was valid. Use default + report. + return chosen, ( + f"unknown price target (checked {', '.join(f'{s}={k!r}' for s, k in candidates)}); " + f"valid: {', '.join(sorted(PRICES))}" + ) + + # Even when we picked a valid key, if the *user* set one and it was + # unknown, surface that. (We only get here if a later candidate resolved, + # so the invalid one was upstream.) + invalid = [(s, k) for s, k in candidates if k not in PRICES and s != "default"] + if invalid: + return chosen, ( + f"unknown price target (set {', '.join(f'{s}={k!r}' for s, k in invalid)}); " + f"valid: {', '.join(sorted(PRICES))}; falling back to `{chosen}`" + ) + return chosen, None + + +def equivalent_cost(usage: dict, price_key: str) -> float: + """USD the measured usage would have billed on `price_key`'s provider. + + `usage` is the dict from `parse_opencode_events` (input/output/reasoning/ + cache_read/cache_write). Builds a `cost_model.Usage` and runs `cost()`. The + pilot's actual provider (headroom/glm-5.2:cloud) reports $0 โ€” this is what + the same tokens would cost on a paid model, so maintainers can budget. + """ + from cost_model import Usage, cost, PRICES # local import: ollama path dep-free + if price_key not in PRICES: + return 0.0 + u = Usage( + uncached_input=(usage.get("input", 0) - usage.get("cache_read", 0)), + cached_input=usage.get("cache_read", 0), + cache_writes=usage.get("cache_write", 0), + output=usage.get("output", 0), + ) + return cost(u, PRICES[price_key]) + + +def format_usage_section( + usage: dict | None, + findings: list[dict], + model: str, + config: dict | None = None, +) -> str: """Render the `## ๐Ÿ”‹ AI usage` block for the review body. Only called when the PR carries the `AI-USAGE` label (and the opencode @@ -205,18 +293,31 @@ def format_usage_section(usage: dict | None, findings: list[dict], model: str) - (input/output/reasoning/cache/cost/steps/duration) plus an ATTRIBUTED per-finding table โ€” one model pass generates all findings, so per-comment counts are an estimate (output split by body weight), clearly labelled. + + The cost lines show TWO numbers because the pilot runs on headroom at + $0/MTok: the `actual` line is what was billed (always $0.00 today), and + the `est. cost on ` line shows what the same measured tokens + would have cost on a paid model โ€” the number a maintainer actually cares + about when budgeting. `config["cost_target"]` / `PRAGENT_PRICE_TARGET` + / `DEFAULT_PRICE_TARGET` (claude-sonnet-5) picks the comparison provider. + Returns "" if `usage` is None. """ if not usage: return "" dur = usage.get("duration_s") dur_s = f"{dur}s" if dur is not None else "?" - cost = usage.get("cost") or 0.0 - cost_s = f"${cost:.4f}" if cost else "$0.00" - cost_note = ( - "(on-network glm-5.2:cloud via headroom โ€” no per-token charge)" - if not cost else "(billed by provider)" + actual = usage.get("cost") or 0.0 + actual_s = f"${actual:.4f}" if actual else "$0.00" + actual_note = ( + "(headroom glm-5.2:cloud โ€” free tier)" + if not actual else "(billed by provider)" ) + price_key, price_err = _resolve_price_target(config) + from cost_model import PRICES # local import keeps ollama path dep-free + eq = equivalent_cost(usage, price_key) + eq_s = f"${eq:.4f}" if eq else "$0.00" + eq_label = PRICES[price_key].name lines = [ "## ๐Ÿ”‹ AI usage", "", @@ -227,7 +328,12 @@ def format_usage_section(usage: dict | None, findings: list[dict], model: str) - f"{usage.get('cache_read', 0)} read / {usage.get('cache_write', 0)} write " f"โ†’ {usage.get('total', 0)} total" ), - f"- est. cost: {cost_s} {cost_note}", + f"- est. cost on **{eq_label}**: {eq_s}" + ( + f" _(price target: `{price_key}`; " + f"{price_err})_" + if price_err else "" + ), + f"- actual: {actual_s} {actual_note}", "- scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) โ€” input tokens include files read beyond the diff", "- per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight)", ] @@ -257,23 +363,38 @@ def build_user_prompt( """Assemble the user prompt: repo config + prior reviews + PR meta + diff.""" parts: list[str] = [] - if config: + eff = effective_config(config) if config else {} + if eff: cfg_lines = [] - if config.get("focus"): - cfg_lines.append("Focus areas: " + ", ".join(config["focus"])) - if config.get("exclude_paths"): - cfg_lines.append("Ignore paths: " + ", ".join(config["exclude_paths"])) - if config.get("languages"): - cfg_lines.append("Languages: " + ", ".join(config["languages"])) - if config.get("instructions"): - cfg_lines.append("Instructions:\n" + str(config["instructions"]).strip()) + if eff.get("focus"): + cfg_lines.append("Focus areas: " + ", ".join(eff["focus"])) + if eff.get("exclude_paths"): + cfg_lines.append("Ignore paths: " + ", ".join(eff["exclude_paths"])) + if eff.get("languages"): + cfg_lines.append("Languages: " + ", ".join(eff["languages"])) + if eff.get("style"): + cfg_lines.append(f"Review style: {eff['style']} " + f"(max {eff['max_findings']} findings, threshold " + f"{eff['severity_threshold']}+)") + if eff.get("patterns", {}).get("allow"): + cfg_lines.append("Allow paths (only these are reviewed): " + + ", ".join(eff["patterns"]["allow"])) + if eff.get("patterns", {}).get("deny"): + cfg_lines.append("Deny paths: " + ", ".join(eff["patterns"]["deny"])) + if eff.get("exclude_tests"): + cfg_lines.append("Skip test files entirely.") + if eff.get("require_tests"): + cfg_lines.append("Flag behavioral changes that don't add a test " + "alongside (added as a `low` finding).") + if eff.get("instructions"): + cfg_lines.append("Instructions:\n" + str(eff["instructions"]).strip()) if cfg_lines: parts.append("## Repo review config (.pr-review.json)\n" + "\n".join(cfg_lines)) if prior_reviews: joined = "\n\n---\n\n".join(prior_reviews) - if len(joined) > 8000: - joined = joined[:8000] + "\nโ€ฆ[prior reviews truncated]" + if len(joined) > 4000: + joined = joined[:4000] + "\nโ€ฆ[prior reviews truncated]" parts.append("## PREVIOUS REVIEWS (already posted โ€” do NOT repeat these points)\n" + joined) parts.append(f"## PR\nTitle: {title or '(none)'}") @@ -638,13 +759,30 @@ def summary_bullets(findings: list[dict]) -> str: CONFIG_MAX_LIST_ITEMS = 32 CONFIG_MAX_ITEM_CHARS = 200 CONFIG_MAX_INSTRUCTIONS_CHARS = 4000 +CONFIG_MAX_PATTERNS_ITEMS = 16 # allow + deny separately, total 32 entries +CONFIG_MAX_FINDINGS = 30 + +STYLES = frozenset(STYLE_DEFAULTS) +SEVERITY_VALUES = frozenset(SEVERITIES) def parse_repo_config(raw: str) -> dict: """Parse a .pr-review.json blob tolerantly. Returns {} on any failure. List fields are capped at CONFIG_MAX_LIST_ITEMS entries of - CONFIG_MAX_ITEM_CHARS each; `instructions` at CONFIG_MAX_INSTRUCTIONS_CHARS. + CONFIG_MAX_ITEM_CHARS each; `instructions` at CONFIG_MAX_INSTRUCTIONS_CHARS; + `patterns.allow` / `patterns.deny` each capped at CONFIG_MAX_PATTERNS_ITEMS + of CONFIG_MAX_ITEM_CHARS. + + Recognised keys (all optional): + focus, exclude_paths, languages, instructions โ€” text steer + style strict|balanced|lenient โ€” default: balanced + severity_threshold low|medium|high|critical โ€” default: per style + max_findings 1..CONFIG_MAX_FINDINGS โ€” default: per style + exclude_tests bool โ€” default: False + require_tests bool โ€” default: False + patterns {allow:[โ€ฆ], deny:[โ€ฆ]} โ€” post-filter globs + cost_target โ€” see equivalent_cost """ if not raw: return {} @@ -654,17 +792,195 @@ def parse_repo_config(raw: str) -> dict: return {} if not isinstance(data, dict): return {} - out = {} - for k in ("focus", "exclude_paths", "languages"): - v = data.get(k) + + def _str_list(v): if isinstance(v, list) and all(isinstance(x, str) for x in v): - out[k] = [x[:CONFIG_MAX_ITEM_CHARS] for x in v[:CONFIG_MAX_LIST_ITEMS]] + return [x[:CONFIG_MAX_ITEM_CHARS] for x in v[:CONFIG_MAX_LIST_ITEMS]] + return None + + out: dict = {} + for k in ("focus", "exclude_paths", "languages"): + s = _str_list(data.get(k)) + if s is not None: + out[k] = s + instr = data.get("instructions") if isinstance(instr, str) and instr.strip(): out["instructions"] = instr.strip()[:CONFIG_MAX_INSTRUCTIONS_CHARS] + + style = data.get("style") + if isinstance(style, str) and style.strip().lower() in STYLES: + out["style"] = style.strip().lower() + + thresh = data.get("severity_threshold") + if isinstance(thresh, str) and thresh.strip().lower() in SEVERITY_VALUES: + out["severity_threshold"] = thresh.strip().lower() + + mf = data.get("max_findings") + if isinstance(mf, int) and not isinstance(mf, bool) and 1 <= mf <= CONFIG_MAX_FINDINGS: + out["max_findings"] = mf + elif isinstance(mf, str) and mf.strip().isdigit(): + n = int(mf.strip()) + if 1 <= n <= CONFIG_MAX_FINDINGS: + out["max_findings"] = n + + for bk in ("exclude_tests", "require_tests"): + if isinstance(data.get(bk), bool): + out[bk] = data[bk] + + pat = data.get("patterns") + if isinstance(pat, dict): + allow = _str_list(pat.get("allow")) + deny = _str_list(pat.get("deny")) + patterns = {} + if allow is not None: + patterns["allow"] = allow[:CONFIG_MAX_PATTERNS_ITEMS] + if deny is not None: + patterns["deny"] = deny[:CONFIG_MAX_PATTERNS_ITEMS] + if patterns: + out["patterns"] = patterns + + ct = data.get("cost_target") + if isinstance(ct, str) and ct.strip(): + out["cost_target"] = ct.strip() + return out +def effective_config(config: dict | None) -> dict: + """Apply STYLE_DEFAULTS for any field the config didn't pin. + + Returns a NEW dict combining the user's `.pr-review.json` (if any) with the + derived `max_findings` / `severity_threshold`. Style itself is preserved + so downstream code can branch on it. + """ + style = (config or {}).get("style", "balanced") + max_findings, severity_threshold = STYLE_DEFAULTS.get(style, STYLE_DEFAULTS["balanced"]) + out = dict(config or {}) + out.setdefault("style", style) + out.setdefault("max_findings", max_findings) + out.setdefault("severity_threshold", severity_threshold) + return out + + +_TEST_PATH_RE = re.compile( + r"(?:^|/)(" + r"[^/]*[Tt]est\.[A-Za-z]+" # FooTest.java / foo_test.py + r"|[^/]*\.[Tt]est\.[A-Za-z]+" # foo.Test.java + r"|[^/]*_test\.py" # foo_test.py + r"|test_[^/]*\.py" # test_foo.py + r"|__tests__/[^/]+" # __tests__/foo.js + r"|[^/]*\.spec\.[A-Za-z]+" # foo.spec.ts + r")$" +) + + +def is_test_path(path: str) -> bool: + """Heuristic: is `path` a test file by name/path convention? + + Conservative โ€” false positives cost real findings; false negatives just + produce one extra line in the summary. Patterns: `FooTest.java`, + `foo_test.py`, `test_foo.py`, `__tests__/foo.js`, `foo.spec.ts`, anything + ending in `.Test.java`. + """ + if not path: + return False + return bool(_TEST_PATH_RE.search(path)) + + +def _glob_to_regex(glob: str) -> re.Pattern: + """Translate a shell-style glob to a compiled regex. + + Supports `*` (any chars except `/`), `**` (any chars including `/`), + `?` (single non-`/` char). Other characters are escaped. Used by + `apply_repo_config` to test `patterns.allow` / `patterns.deny` globs. + """ + out = [] + i = 0 + while i < len(glob): + c = glob[i] + if c == "*": + if i + 1 < len(glob) and glob[i + 1] == "*": + out.append(".*") + i += 2 + # swallow a following `/` so `**/x` and `x/**/y` behave + if i < len(glob) and glob[i] == "/": + i += 1 + continue + out.append("[^/]*") + elif c == "?": + out.append("[^/]") + else: + out.append(re.escape(c)) + i += 1 + return re.compile("^" + "".join(out) + "$") + + +def apply_repo_config( + findings: list[dict], + config: dict | None, + changed_paths: list[str] | None = None, +) -> tuple[list[dict], list[dict]]: + """Filter + cap findings per `.pr-review.json` rules. Returns (kept, dropped). + + Filters applied (in order): + 1. `exclude_tests` + test-path heuristic โ†’ drop test files + 2. `exclude_paths` glob match โ†’ drop matched paths + 3. `patterns.deny` glob match โ†’ drop matched paths + 4. `patterns.allow` (if non-empty) โ†’ keep ONLY matched paths + 5. `severity_threshold` โ†’ drop below threshold + 6. `max_findings` โ†’ keep first N (highest-severity-first) + 7. `require_tests` โ†’ append a low-severity finding + if changed paths include non-test files but no test files changed + alongside them (caller passes `changed_paths` from the brief). + """ + eff = effective_config(config) + keep: list[dict] = [] + drop: list[dict] = [] + deny_globs = [_glob_to_regex(g) for g in (eff.get("patterns", {}) or {}).get("deny", [])] + allow_globs = [_glob_to_regex(g) for g in (eff.get("patterns", {}) or {}).get("allow", [])] + deny_path_globs = [_glob_to_regex(g) for g in eff.get("exclude_paths", [])] + threshold_rank = SEVERITY_RANK[eff["severity_threshold"]] + + for f in findings: + path = f.get("path", "") + if eff.get("exclude_tests") and is_test_path(path): + drop.append(f); continue + if any(rx.search(path) for rx in deny_path_globs): + drop.append(f); continue + if any(rx.search(path) for rx in deny_globs): + drop.append(f); continue + if allow_globs and not any(rx.search(path) for rx in allow_globs): + drop.append(f); continue + sev_rank = SEVERITY_RANK.get(f.get("severity", "low"), 0) + if sev_rank < threshold_rank: + drop.append(f); continue + keep.append(f) + + cap = eff["max_findings"] + if len(keep) > cap: + dropped = keep[cap:] + keep = keep[:cap] + drop.extend(dropped) + + if eff.get("require_tests") and changed_paths is not None: + non_test = [p for p in changed_paths if not is_test_path(p)] + any_test = any(is_test_path(p) for p in changed_paths) + if non_test and not any_test: + keep.append({ + "severity": "low", + "path": non_test[0], + "line": 1, + "problem": "no test file changed alongside this behavioral change (require_tests=true)", + "fix": "add a unit test exercising the changed branch", + "suggestion": "", + "reference": "", + "_config_synthetic": True, + }) + + return keep, drop + + def reviewed_shas(reviews: list[dict]) -> set[str]: """Pull every `` marker out of a PR's reviews.""" shas: set[str] = set() @@ -693,6 +1009,29 @@ def prior_review_bodies(reviews: list[dict], current_sha: str, limit: int = 6) - return out[:limit] +def compact_prior_reviews(prior_bodies: list[str]) -> list[str]: + """Squeeze prior review bodies down to just the finding bullets. + + Each prior review's prose ("this PR adds eval() โ€” risky") is noise when the + model already has the diff; the only thing it needs to *not repeat* is what + was already flagged. We extract lines matching `-\\s*\\*\\*[SEV]\\*\\*` + plus their directly-attached location reference (so `[CRITICAL]` stays + anchored to `path:line`), drop the rest, and return one bullet-list per + prior review. A prior review that had no parseable findings becomes an + empty string and is dropped. + + Local import keeps the ollama path dep-free (extract_finding_bullets lives + in pilot/diff_compress.py). + """ + from diff_compress import extract_finding_bullets + out = [] + for body in prior_bodies or []: + bullets = extract_finding_bullets(body) + if bullets: + out.append("\n".join(bullets)) + return out + + # --------------------------------------------------------------------------- # Network helpers # --------------------------------------------------------------------------- @@ -911,13 +1250,35 @@ def review_pr( print(f"pragent: {repo}#{index} sha={sha[:8]} already reviewed, skipping", flush=True) return True - diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars) - if not diff.strip(): + raw_diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars) + if not raw_diff.strip(): post_review(api, repo, index, token, format_review_body("No diff content to review.", model, sha)) return True config = fetch_repo_config(api, repo, token, ref=base_ref) - prior = prior_review_bodies(reviews, sha) + prior = compact_prior_reviews(prior_review_bodies(reviews, sha)) + + # Trim the diff to +/- hunks plus a narrow context window. The agent + # resends the brief prefix every step, so a 25k-char diff becomes + # 25k ร— 30-step ร— cached-after-step-1 = hundreds of thousands of input + # tokens. Default context=1: enough for the reviewer to see what an + # added line is replacing; the full file is on disk in the workdir + # anyway, so anything more is reading the diff twice. Tunable via + # PRAGENT_DIFF_CONTEXT (0 = +/- only; -1 = disable compression). + from diff_compress import compress_diff + ctx = int(os.environ.get("PRAGENT_DIFF_CONTEXT", "1")) + if ctx < 0: + diff = raw_diff + compression_note = "" + else: + diff, orig_chars, kept_chars = compress_diff(raw_diff, context=ctx) + if kept_chars < orig_chars: + compression_note = ( + f"\n\n> _diff compressed: {orig_chars:,} โ†’ {kept_chars:,} chars " + f"(context={ctx}; PRAGENT_DIFF_CONTEXT to tune)_" + ) + else: + compression_note = "" engine = os.environ.get("PRAGENT_ENGINE", "opencode").strip().lower() review_summary = "" @@ -934,6 +1295,7 @@ def review_pr( api=api, repo=repo, index=index, sha=sha, token=token, title=title, body=body, diff=diff, config=config, prior_reviews=prior, model=oc_model, + compression_note=compression_note, ) review_summary, findings = parse_review_output(stdout) if not findings and not review_summary: @@ -949,24 +1311,48 @@ def review_pr( salvaged = salvage_summary(stdout) usage_section = "" if report_usage and usage: - usage_section = format_usage_section(usage, [], model) + usage_section = format_usage_section(usage, [], model, config=config) post_review(api, repo, index, token, format_review_body( salvaged or "AI review produced no parseable output.", model, sha, usage_section=usage_section)) return True else: - user_prompt = build_user_prompt(title, body, diff, config, prior) + user_prompt = build_user_prompt(title, body + compression_note, diff, config, prior) raw_findings = call_model(ollama_url, model, SYSTEM_PROMPT, user_prompt, max_tokens) findings = parse_findings(raw_findings) usage = None + # Filter / cap findings per `.pr-review.json` (style, threshold, max, + # patterns, exclude_tests). Without this every config knob would be a + # no-op โ€” the agent has no view into the config beyond instructions. + # The synthetic require_tests finding (if any) is appended here. + try: + changed_paths = sorted({ + f.get("path", "") + for f in findings + if f.get("path") + }) + except Exception: + changed_paths = [] + kept, _dropped = apply_repo_config(findings, config, changed_paths=changed_paths) + findings = kept + if _dropped: + print( + f"pragent: {repo}#{index} sha={sha[:8]} filtered " + f"{len(_dropped)} finding(s) per .pr-review.json " + f"(style={(config or {}).get('style', 'balanced')}, " + f"threshold={(config or {}).get('severity_threshold', '?')}, " + f"max={len(findings)})", + flush=True, + ) + # Attribute output tokens to each finding (mutates finding dicts) so # inline comments + the usage table can show a per-comment estimate. # Only meaningful when we have measured usage AND the PR asked for it. usage_section = "" if report_usage and usage and usage.get("output"): compute_attribution(findings, usage["output"]) - usage_section = format_usage_section(usage, findings, model) + usage_section = format_usage_section(usage, findings, model, config=config) anchors = parse_diff_anchors(diff) anchored, unanchored = split_findings(findings, anchors) diff --git a/pilot/opencode_review.py b/pilot/opencode_review.py index 5c1832c..2cadbd5 100644 --- a/pilot/opencode_review.py +++ b/pilot/opencode_review.py @@ -286,6 +286,7 @@ def write_brief( diff: str, config: dict | None, prior_reviews: list[str] | None, + compression_note: str = "", ) -> str: """Render `.pragent/brief.md` in the workdir. Returns the path written.""" path = os.path.join(workdir, ".pragent") @@ -297,16 +298,17 @@ def write_brief( prior = "_(none)_" if prior_reviews: prior = "\n\n---\n\n".join(prior_reviews) - if len(prior) > 8000: - prior = prior[:8000] + "\nโ€ฆ[prior reviews truncated]" + if len(prior) > 4000: + prior = prior[:4000] + "\nโ€ฆ[prior reviews truncated]" files = changed_files(diff) files_block = "\n".join(f"- `{p}`" for p in files) if files else "_(none)_" + desc_block = ((description or "").strip() or "_(none)_") + compression_note content = _BRIEF_TEMPLATE.format( repo=repo or "?", index=index or "?", sha=sha or "?", title=title or "(none)", - description=description.strip() or "_(none)_", + description=desc_block, changed_files=files_block, config=cfg, prior=prior, @@ -672,6 +674,7 @@ def run( config: dict | None, prior_reviews: list[str] | None, model: str, + compression_note: str = "", ) -> tuple[str, dict | None]: """End-to-end: checkout archive โ†’ brief โ†’ drop factory โ†’ opencode โ†’ (text, usage). @@ -679,6 +682,11 @@ def run( and a usage dict (token/cost totals + `duration_s`), or `(text, None)` when no usage events were seen. Raises on any failure; the caller (`review_pr`) fails open. The workdir is removed unless PRAGENT_KEEP_WORK is set. + + `compression_note`: a small markdown block to append to the brief's PR + description (e.g. "diff compressed: 25k โ†’ 12k chars"). Empty string by + default. Appended AFTER the untrusted-data fence so the agent reads it as + guidance, not author input. """ os.makedirs(WORK_ROOT, exist_ok=True) workdir = tempfile.mkdtemp(prefix=f"{repo.replace('/', '_')}-{sha[:8]}-", dir=WORK_ROOT) @@ -697,6 +705,7 @@ def run( workdir, repo=repo, index=index, sha=sha, title=title, description=body, diff=diff, config=config, prior_reviews=prior_reviews, + compression_note=compression_note, ) drop_factory(workdir) text, usage = run_opencode(workdir, model) diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index 0c412c6..a6bf7b8 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -779,3 +779,244 @@ def test_salvage_summary_empty_when_nothing_to_salvage(): assert ai_review.salvage_summary("") == "" assert ai_review.salvage_summary(" \n ") == "" assert ai_review.salvage_summary("```json\n{}\n```") == "" + + +# --------------------------------------------------------------------------- +# equivalent_cost + format_usage_section equivalent-provider line +# --------------------------------------------------------------------------- + + +def test_equivalent_cost_matches_cost_model(): + usage = {"input": 1_000_000, "output": 0, "cache_read": 0, "cache_write": 0} + eq = ai_review.equivalent_cost(usage, "claude-sonnet-5") + # Sonnet 5 input is $2/MTok, so 1M input = $2.00 exactly. + assert abs(eq - 2.0) < 1e-9 + + +def test_equivalent_cost_unknown_key_returns_zero(): + assert ai_review.equivalent_cost({"input": 100}, "bogus") == 0.0 + + +def test_format_usage_section_shows_equivalent_provider_cost(): + usage = {"input": 200000, "output": 4000, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 204000, + "cost": 0.0, "steps": 6, "duration_s": 100.0} + sec = ai_review.format_usage_section(usage, [], "glm-5.2:cloud") + # Two cost lines now: an equivalent (default Sonnet 5) AND the $0 actual. + assert "## ๐Ÿ”‹ AI usage" in sec + assert "est. cost on **Claude Sonnet 5**" in sec + assert "actual: $0.00" in sec + assert "free tier" in sec + # Equivalent should be > 0 for non-trivial token counts. + assert "$0.00" in sec # the actual line + # And a non-zero one for the equivalent. + import re + cost_lines = [ln for ln in sec.splitlines() if "cost on" in ln] + assert len(cost_lines) == 1 + assert re.search(r"\$\d", cost_lines[0]) is not None + assert "$0.00" not in cost_lines[0] + + +def test_format_usage_section_honors_cost_target(monkeypatch): + monkeypatch.setenv("PRAGENT_PRICE_TARGET", "claude-haiku-4-5") + usage = {"input": 1000, "output": 100, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 1100, + "cost": 0.0, "steps": 1, "duration_s": 5.0} + sec = ai_review.format_usage_section(usage, [], "glm-5.2:cloud") + assert "Claude Haiku 4.5" in sec + # 1k * $1/MTok + 100 * $5/MTok = 0.001 + 0.0005 = $0.0015 + assert "$0.0015" in sec + + +def test_format_usage_section_respects_repo_config_cost_target(monkeypatch): + monkeypatch.delenv("PRAGENT_PRICE_TARGET", raising=False) + usage = {"input": 1000, "output": 100, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 1100, + "cost": 0.0, "steps": 1, "duration_s": 5.0} + sec = ai_review.format_usage_section( + usage, [], "glm-5.2:cloud", config={"cost_target": "claude-opus-5"} + ) + assert "Claude Opus 5" in sec + # Opus 5 = $5/MTok input + $25/MTok output โ†’ 1000*5e-6 + 100*25e-6 = 0.0075 + assert "$0.0075" in sec + + +def test_format_usage_section_reports_unknown_price_target(): + usage = {"input": 100, "output": 100, "reasoning": 0, + "cache_read": 0, "cache_write": 0, "total": 200, + "cost": 0.0, "steps": 1, "duration_s": 1.0} + sec = ai_review.format_usage_section( + usage, [], "glm-5.2:cloud", config={"cost_target": "bogus-model"} + ) + # Falls back to default + surfaces the error in the line. + assert "Claude Sonnet 5" in sec + assert "unknown price target" in sec + assert "bogus-model" in sec + + +# --------------------------------------------------------------------------- +# parse_repo_config โ€” extended schema +# --------------------------------------------------------------------------- + + +def test_parse_repo_config_new_fields_all_valid(): + raw = json.dumps({ + "focus": ["security"], + "style": "strict", + "severity_threshold": "high", + "max_findings": 5, + "exclude_tests": True, + "require_tests": True, + "patterns": {"allow": ["src/**"], "deny": ["**/*.test.ts"]}, + "cost_target": "claude-opus-5", + }) + c = ai_review.parse_repo_config(raw) + assert c["style"] == "strict" + assert c["severity_threshold"] == "high" + assert c["max_findings"] == 5 + assert c["exclude_tests"] is True + assert c["require_tests"] is True + assert c["patterns"]["allow"] == ["src/**"] + assert c["patterns"]["deny"] == ["**/*.test.ts"] + assert c["cost_target"] == "claude-opus-5" + + +def test_parse_repo_config_rejects_bad_style_and_threshold(): + c = ai_review.parse_repo_config(json.dumps({"style": "wild", "severity_threshold": "meh"})) + assert "style" not in c + assert "severity_threshold" not in c + + +def test_parse_repo_config_caps_max_findings(): + c1 = ai_review.parse_repo_config(json.dumps({"max_findings": 0})) + c2 = ai_review.parse_repo_config(json.dumps({"max_findings": 999})) + c3 = ai_review.parse_repo_config(json.dumps({"max_findings": "12"})) + assert "max_findings" not in c1 # 0 invalid + assert "max_findings" not in c2 # > 30 invalid + assert c3["max_findings"] == 12 # numeric string accepted + + +def test_parse_repo_config_caps_patterns(): + raw = json.dumps({ + "patterns": {"allow": [f"a{i}" for i in range(20)], "deny": [f"d{i}" for i in range(20)]} + }) + c = ai_review.parse_repo_config(raw) + assert len(c["patterns"]["allow"]) == ai_review.CONFIG_MAX_PATTERNS_ITEMS + assert len(c["patterns"]["deny"]) == ai_review.CONFIG_MAX_PATTERNS_ITEMS + + +def test_effective_config_applies_style_defaults(): + eff = ai_review.effective_config({"focus": ["security"]}) + assert eff["style"] == "balanced" + assert eff["max_findings"] == 12 + assert eff["severity_threshold"] == "medium" + assert eff["focus"] == ["security"] + + +def test_effective_config_style_overrides_fields(): + eff = ai_review.effective_config({"style": "strict"}) + assert eff["max_findings"] == 5 + assert eff["severity_threshold"] == "high" + + +# --------------------------------------------------------------------------- +# apply_repo_config โ€” filter findings +# --------------------------------------------------------------------------- + + +_FINDINGS = [ + {"severity": "critical", "path": "src/main.py", "line": 1, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "high", "path": "src/main.py", "line": 5, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "medium", "path": "src/main.py", "line": 9, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "low", "path": "src/main.py", "line": 13, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "high", "path": "src/FooTest.java", "line": 22, "problem": "p", "fix": "f", "suggestion": ""}, + {"severity": "medium", "path": "src/app.test.ts", "line": 7, "problem": "p", "fix": "f", "suggestion": ""}, +] + + +def test_apply_repo_config_severity_threshold(): + kept, dropped = ai_review.apply_repo_config(_FINDINGS, {"severity_threshold": "high"}) + assert len(kept) == 3 # critical + 2 highs (main.py + FooTest.java) + assert all(f["severity"] in ("critical", "high") for f in kept) + assert len(dropped) == 3 + + +def test_apply_repo_config_exclude_tests_drops_test_files(): + kept, dropped = ai_review.apply_repo_config(_FINDINGS, {"exclude_tests": True}) + paths = {f["path"] for f in kept} + assert "src/FooTest.java" not in paths + assert "src/app.test.ts" not in paths + + +def test_apply_repo_config_patterns_deny_drops_matching(): + cfg = {"patterns": {"deny": ["src/main.py"]}} + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg) + paths = {f["path"] for f in kept} + assert "src/main.py" not in paths + + +def test_apply_repo_config_patterns_allow_keeps_only_matching(): + cfg = {"patterns": {"allow": ["src/main.py"]}} + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg) + paths = {f["path"] for f in kept} + assert paths == {"src/main.py"} + + +def test_apply_repo_config_max_findings_caps(): + kept, dropped = ai_review.apply_repo_config(_FINDINGS, {"max_findings": 2}) + assert len(kept) == 2 + # Highest-severity first (critical, then high) + assert kept[0]["severity"] == "critical" + assert kept[1]["severity"] == "high" + + +def test_apply_repo_config_exclude_paths_glob(): + cfg = {"exclude_paths": ["src/main.py"]} + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg) + assert "src/main.py" not in {f["path"] for f in kept} + + +def test_apply_repo_config_require_tests_synthetic_finding(): + cfg = {"require_tests": True} + changed = ["src/main.py", "src/lib.ts"] + kept, dropped = ai_review.apply_repo_config([], cfg, changed_paths=changed) + assert any(f.get("_config_synthetic") for f in kept) + + +def test_apply_repo_config_require_tests_no_synthetic_when_tests_present(): + cfg = {"require_tests": True} + changed = ["src/main.py", "src/main_test.py"] + kept, dropped = ai_review.apply_repo_config(_FINDINGS, cfg, changed_paths=changed) + assert not any(f.get("_config_synthetic") for f in kept) + + +def test_is_test_path_recognises_common_patterns(): + assert ai_review.is_test_path("src/FooTest.java") + assert ai_review.is_test_path("src/foo.test.ts") + assert ai_review.is_test_path("tests/foo_test.py") + assert ai_review.is_test_path("test_foo.py") + assert ai_review.is_test_path("packages/app/__tests__/foo.js") + assert not ai_review.is_test_path("src/main.py") + assert not ai_review.is_test_path("src/testing.py") # "testing" โ‰  "test_" + + +# --------------------------------------------------------------------------- +# compact_prior_reviews +# --------------------------------------------------------------------------- + + +def test_compact_prior_reviews_drops_prose_keeps_bullets(): + bodies = [ + "๐Ÿค– AI Review ยท m ยท `abc`\n\nLong prose.\n\n- **[HIGH]** `a.py:1` โ€” bug.\n- **[LOW]** `b.go:2` โ€” nit.\n\n_2 inline comments posted._\n", + "Just chatter, no findings.", + ] + out = ai_review.compact_prior_reviews(bodies) + assert len(out) == 1 + assert "HIGH" in out[0] and "a.py:1" in out[0] + assert "Long prose." not in out[0] + assert "inline comments posted" not in out[0] + + +def test_compact_prior_reviews_empty_and_none(): + assert ai_review.compact_prior_reviews([]) == [] + assert ai_review.compact_prior_reviews(None) == []