diff --git a/pilot/ai_review.py b/pilot/ai_review.py index 21204a6..a55107a 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -273,12 +273,18 @@ def format_review_body( walkthrough: list[str] | None = None, risk_verdict: str = "", test_coverage: str = "", + static_message: str = "", + ) -> str: """Format the posted review summary body. Layout (per the operator's format guide): * Header line (``🤖 AI Review …``) including the merge-confidence badge. + * Optional static banner (``> {static_message}``) — repo-wide call-out + from `.pr-review.json:static_message`, placed under the header so + every reviewer sees it on every review without scrolling. + * **Summary of Changes** — 2–4 bullets of what the PR introduces (`summary_changes`); falls back to the opencode prose `summary` if the agent didn't emit the list. @@ -317,6 +323,11 @@ def format_review_body( ) parts: list[str] = [header] + # Optional free-text banner. Rendered as a Markdown blockquote immediately + # after the header — front-of-mind for any maintainer scanning the review. + if static_message and static_message.strip(): + parts.append(f"> {static_message.strip()}") + # --- Summary of Changes --- sc = list(summary_changes or []) if not sc and summary: @@ -467,6 +478,32 @@ def _resolve_price_target(config: dict | None) -> tuple[str, str | None]: return chosen, None +def _resolve_display_model(base_model: str, config: dict | None) -> str: + """Resolve the *display* model for one review. + + Precedence (highest first): + 1. `OPENCODE_MODEL` env var — operator override, used as-is (already a + provider-prefixed opencode ref). + 2. `.pr-review.json:model` — per-repo override. Already validated + against `cost_model.PRICES` by `parse_repo_config`, so a bare key + like `claude-sonnet-5` is safe to use as the opencode ref AND the + REVIEW_HEADER label. + 3. Default — `f"headroom/{base_model}"` where `base_model` is the bare + `OLLAMA_MODEL` (e.g. `"MiniMax-M2.7" → "headroom/MiniMax-M2.7"`). + + The same value flows to every consumer (opencode subprocess, REVIEW_HEADER, + cost-line parenthetical) so reviewers never see a mix of `glm-5.2:cloud` + and the routed model in one body. + """ + env = os.environ.get("OPENCODE_MODEL") + if env: + return env + cfg_model = (config or {}).get("model") + if isinstance(cfg_model, str) and cfg_model.strip(): + return cfg_model.strip() + return f"headroom/{base_model}" + + def equivalent_cost(usage: dict, price_key: str) -> float: """USD the measured usage would have billed on `price_key`'s provider. @@ -1195,7 +1232,11 @@ def _render_collapsible_usage(usage: dict | None, model: str, config: dict | Non would bill on mainstream paid APIs (configurable via `compare_against`, defaulting to ``DEFAULT_COMPARE_AGAINST``). The row matching `cost_target` is bolded so the price target stands out. The whole table is omitted when - every row would be $0 (no work done). + every row would be $0 (no work done). The `actual` parenthetical clause + reflects the *actually-routed* model (`model` arg, resolved by caller from + `OPENCODE_MODEL` env or `headroom/{OLLAMA_MODEL}`) — cost == 0 → "free + tier", nonzero → "billed". + """ if not usage: return "" @@ -1203,7 +1244,7 @@ def _render_collapsible_usage(usage: dict | None, model: str, config: dict | Non 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 "" + actual_note = f" ({model} — {'free tier' if not actual else 'billed'})" cost_target, price_err = _resolve_price_target(config) if price_err: # Surface config typos loudly but do not pollute the posted summary @@ -1232,6 +1273,7 @@ def _render_collapsible_usage(usage: dict | None, model: str, config: dict | Non cost_str = f"${c:.4f}" if c < 0.01 else f"${c:.2f}" bold = "**" if key == cost_target else "" eq_rows.append(f"| {bold}{label}{bold} | {cost_str} |") + in_tok = usage.get("input", 0) out_tok = usage.get("output", 0) reason_tok = usage.get("reasoning", 0) @@ -1288,6 +1330,7 @@ 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 +CONFIG_MAX_STATIC_MESSAGE_CHARS = 400 # free-text banner, mirror of instructions STYLES = frozenset(STYLE_DEFAULTS) SEVERITY_VALUES = frozenset(SEVERITIES) @@ -1303,12 +1346,14 @@ def parse_repo_config(raw: str) -> dict: Recognised keys (all optional): focus, exclude_paths, languages, instructions — text steer + static_message ≤ CONFIG_MAX_STATIC_MESSAGE_CHARS — banner under header 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 + model — per-repo override cost_target — see equivalent_cost additional_context_urls list[str] (≤ 8) — see fetch_additional_context """ @@ -1336,6 +1381,10 @@ def parse_repo_config(raw: str) -> dict: if isinstance(instr, str) and instr.strip(): out["instructions"] = instr.strip()[:CONFIG_MAX_INSTRUCTIONS_CHARS] + sm = data.get("static_message") + if isinstance(sm, str) and sm.strip(): + out["static_message"] = sm.strip()[:CONFIG_MAX_STATIC_MESSAGE_CHARS] + style = data.get("style") if isinstance(style, str) and style.strip().lower() in STYLES: out["style"] = style.strip().lower() @@ -1372,6 +1421,26 @@ def parse_repo_config(raw: str) -> dict: if isinstance(ct, str) and ct.strip(): out["cost_target"] = ct.strip() + # Per-repo model override. Validated against cost_model.PRICES so the value + # is usable both as the opencode subprocess ref and as the REVIEW_HEADER + # label (see _resolve_display_model precedence). Unknown values are dropped + # with a stderr pointer to the valid set — silently ignoring would mask + # typos from repo admins. + raw_model = data.get("model") + if raw_model is not None: + if isinstance(raw_model, str) and raw_model.strip(): + from cost_model import PRICES # lazy: ollama path dep-free + candidate = raw_model.strip() + if candidate in PRICES: + out["model"] = candidate + else: + print( + f"pragent: .pr-review.json:model={candidate!r} not in " + f"cost_model.PRICES (valid: {', '.join(sorted(PRICES))}); " + f"dropping", + file=sys.stderr, flush=True, + ) + acu = data.get("additional_context_urls") if isinstance(acu, list): urls: list[str] = [] @@ -2023,6 +2092,13 @@ def review_pr( Both the CI `run()` entry point and the central webhook server call this. """ try: + # Pre-compute a *fallback* display name for the early-exit paths + # (already-reviewed dedupe skip, no-diff-content). We re-resolve + # properly after `.pr-review.json` is loaded further down — that + # version honours `OPENCODE_MODEL` env > `.pr-review.json:model` > + # this fallback. + display_model = f"headroom/{model}" + reviews = fetch_existing_reviews(api, repo, index, token) # Dedupe: already reviewed this exact commit -> nothing to do. if sha and sha in reviewed_shas(reviews): @@ -2031,12 +2107,18 @@ def review_pr( 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)) + post_review(api, repo, index, token, format_review_body("No diff content to review.", display_model, sha)) return True config = fetch_repo_config(api, repo, token, ref=base_ref) prior = compact_prior_reviews(prior_review_bodies(reviews, sha)) + # Re-resolve display_model now that .pr-review.json is available — + # per-repo override (`.pr-review.json:model`) takes precedence over + # the bare OLLAMA_MODEL fallback, with OPENCODE_MODEL env still + # winning above both (see `_resolve_display_model`). + display_model = _resolve_display_model(model, config) + # 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 @@ -2070,10 +2152,9 @@ def review_pr( # the brief, and the pragent agent factory; returns stdout with a # summary + findings JSON. We parse + anchor + post here. import opencode_review # local import keeps the ollama path dep-free - # opencode wants a provider-prefixed model ref (headroom/glm-5.2:cloud); - # `model` here is the bare id (OLLAMA_MODEL). OPENCODE_MODEL overrides - # with the full ref; otherwise we prefix the configured provider. - oc_model = os.environ.get("OPENCODE_MODEL") or f"headroom/{model}" + # Reuse the display_model resolved above for the subprocess — same + # provider-prefixed ref goes to the engine and into the review body. + oc_model = display_model # Multi-lens fan-out: when the repo declared `reviewers[]` (or the # operator pinned PRAGENT_REVIEWERS=1), spawn one opencode subprocess # per lens in parallel and synthesize. Falls through to the legacy @@ -2109,10 +2190,11 @@ def review_pr( file=sys.stderr, flush=True, ) salvaged = salvage_summary(stdout) - usage_section = _render_collapsible_usage(usage, model, config=config) if usage else "" + usage_section = _render_collapsible_usage(usage, display_model, config=config) if usage else "" post_review(api, repo, index, token, format_review_body( salvaged or "AI review produced no parseable output.", - model, sha, usage_section=usage_section)) + display_model, sha, usage_section=usage_section, + static_message=(config or {}).get("static_message", ""))) return True else: user_prompt = build_user_prompt(title, body + compression_note, diff, config, prior, additional_context) @@ -2157,7 +2239,7 @@ def review_pr( # estimates. Only meaningful when we have measured usage. if usage and usage.get("output"): compute_attribution(findings, usage["output"]) - usage_section = _render_collapsible_usage(usage, model, config=config) if usage else "" + usage_section = _render_collapsible_usage(usage, display_model, config=config) if usage else "" # Anchor against the RAW diff, never the compressed one. Compression # drops context lines, so a finding on a line that survived in the file @@ -2184,7 +2266,7 @@ def review_pr( # reach this call. confidence = merge_confidence(findings, multi_lens_observed=multi_lens) summary_body = format_review_body( - "\n\n".join(summary_parts), model, sha, + "\n\n".join(summary_parts), display_model, sha, summary=review_summary, usage_section=usage_section, summary_changes=summary_changes, @@ -2192,6 +2274,7 @@ def review_pr( findings_for_table=findings, inline_count=len(anchored), confidence=confidence, + static_message=(config or {}).get("static_message", ""), ) post_inline_review(api, repo, index, token, summary_body, anchored) @@ -2203,7 +2286,7 @@ def review_pr( return True except Exception as e: # fail-open try: - post_review(api, repo, index, token, format_review_body(f"⚠️ AI review failed: {e}", model, sha)) + post_review(api, repo, index, token, format_review_body(f"⚠️ AI review failed: {e}", display_model, sha)) except Exception as e2: print(f"pragent: could not post failure note: {e2}", file=sys.stderr) print(f"pragent: review failed: {e}", file=sys.stderr) diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index c2245dc..9f6ccaa 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -410,6 +410,81 @@ def test_parse_repo_config_partial_and_bad(): assert parse_repo_config('{"instructions":" "}') == {"enabled": False} +def test_parse_repo_config_reads_static_message(): + cfg = parse_repo_config(json.dumps({"static_message": " NOTE: this repo is in maintenance mode "})) + assert cfg.get("static_message") == "NOTE: this repo is in maintenance mode" + assert cfg.get("enabled") is False + + +def test_parse_repo_config_static_message_caps_length(): + long_text = "x" * 9999 + cfg = parse_repo_config(json.dumps({"static_message": long_text})) + assert "static_message" in cfg + assert len(cfg["static_message"]) <= 400 + + +def test_parse_repo_config_static_message_ignores_blank(): + assert "static_message" not in parse_repo_config(json.dumps({"static_message": " "})) + assert "static_message" not in parse_repo_config(json.dumps({"static_message": ""})) + assert "static_message" not in parse_repo_config(json.dumps({"static_message": 42})) + + +def test_parse_repo_config_reads_model_override(): + # Per-repo override is validated against cost_model.PRICES. Only keys + # the cost model knows about can override the review engine. + cfg = parse_repo_config(json.dumps({"model": "claude-sonnet-5"})) + assert cfg.get("model") == "claude-sonnet-5" + + +def test_parse_repo_config_rejects_unknown_model(capsys): + cfg = parse_repo_config(json.dumps({"model": "not-in-prices"})) + assert "model" not in cfg + # Repos that pin a typo should get a stderr hint pointing at the valid set. + err = capsys.readouterr().err + assert "model" in err.lower() or "prices" in err.lower() or "unknown" in err.lower() + + +def test_parse_repo_config_model_must_be_string(): + assert "model" not in parse_repo_config(json.dumps({"model": 42})) + assert "model" not in parse_repo_config(json.dumps({"model": []})) + assert "model" not in parse_repo_config(json.dumps({"model": None})) + + +def test_resolve_display_model_precedence(monkeypatch): + # Order is OPENCODE_MODEL env > config['model'] > headroom/{base}. + monkeypatch.delenv("OPENCODE_MODEL", raising=False) + # 1. No env, no config → headroom/ + assert ai_review._resolve_display_model("MiniMax-M2.7", None) == "headroom/MiniMax-M2.7" + assert ai_review._resolve_display_model("MiniMax-M2.7", {}) == "headroom/MiniMax-M2.7" + # 2. No env, config has model → use config model as-is (already a known key) + assert ( + ai_review._resolve_display_model("MiniMax-M2.7", {"model": "claude-sonnet-5"}) + == "claude-sonnet-5" + ) + # 3. Env wins over config + monkeypatch.setenv("OPENCODE_MODEL", "headroom/MiniMax-M2.7") + assert ( + ai_review._resolve_display_model("MiniMax-M2.7", {"model": "claude-sonnet-5"}) + == "headroom/MiniMax-M2.7" + ) + # 4. Env alone, no config + monkeypatch.delenv("OPENCODE_MODEL") + assert ai_review._resolve_display_model("x", {}) == "headroom/x" + + +def test_format_review_body_uses_override_for_cost_paren(): + # End-to-end sanity: when the caller passes the resolved override as the + # `model` arg to format_review_body, both the header AND the cost line + # show the override — i.e. callers DO substitute the resolved display + # name into both the opencode subprocess ref and the review body. + body = format_review_body( + "- [high] x:1 — bug. fix.", "claude-sonnet-5", "abcdef1234567890", + ) + assert "claude-sonnet-5" in body + assert "MiniMax-M2.7" not in body # the base didn't leak through + assert "🤖" in body # header rendered + + # --------------------------------------------------------------------------- # dedupe / prior-context parsing # --------------------------------------------------------------------------- @@ -789,6 +864,39 @@ def test_render_collapsible_usage_cost_nonzero_drops_free_tier_note(): "cache_write": 0, "total": 10, "cost": 0.0123, "steps": 1, "duration_s": 1.0} sec = _render_collapsible_usage(usage, "m", config=None) assert "$0.0123" in sec + # Was hardcoded "free tier" previously; now says "billed" since cost > 0. + assert "billed" in sec + assert "free tier" not in sec + + +def test_render_collapsible_usage_uses_passed_model_for_free_tier_clause(): + # Regression: the cost parenthetical must reflect the actually-routed model, + # not a stale hardcoded `headroom glm-5.2:cloud` literal that predates the + # MiniMax / Anthropic switch. + 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 = _render_collapsible_usage(usage, "MiniMax-M2.7", config=None) + # The parenthetical clause is "( — free tier)" — a model name MUST + # sit immediately before "— free tier". + assert "(MiniMax-M2.7 — free tier)" in sec + # And the stale hardcoded model name must no longer appear anywhere. + assert "glm-5.2:cloud" not in sec + + +def test_render_collapsible_usage_full_provider_prefix_in_display(): + # When the caller has resolved a provider-prefixed model ref (the opencode + # subprocess path), the parenthetical should mirror that verbatim. + 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 = _render_collapsible_usage(usage, "headroom/MiniMax-M2.7", config=None) + assert "(headroom/MiniMax-M2.7 — free tier)" in sec + + +def test_render_collapsible_usage_nonzero_cost_says_billed(): + usage = {"input": 10, "output": 0, "reasoning": 0, "cache_read": 0, + "cache_write": 0, "total": 10, "cost": 0.123, "steps": 1, "duration_s": 1.0} + sec = _render_collapsible_usage(usage, "MiniMax-M2.7", config=None) + assert "(MiniMax-M2.7 — billed)" in sec assert "free tier" not in sec @@ -808,6 +916,22 @@ def test_format_review_body_no_usage_section_omitted(): assert "AI usage" not in body +def test_format_review_body_renders_static_message_banner(): + body = format_review_body( + "", "glm-5.2:cloud", "abcdef1234567890", + static_message="NOTE: this repo is in maintenance mode.", + ) + assert "> NOTE: this repo is in maintenance mode." in body + # Banner sits under the header and above the rest of the body. + assert body.index("NOTE") > body.index("🤖") + assert body.index("NOTE") < body.index("### Summary of Changes") + + +def test_format_review_body_omits_static_message_when_blank(): + body = format_review_body("", "glm-5.2:cloud", "abcdef1234567890") + assert "> " not in body + + def test_format_review_body_with_summary_changes_and_risks(): body = format_review_body( "", "glm-5.2:cloud", "abcdef1234567890", @@ -1141,7 +1265,11 @@ def test_usage_block_shows_equivalent_provider_cost(): # grok-4.5; cost_target defaults to sonnet-5 (bolded). assert "🔋 AI Usage & Run Details" in sec assert "**Actual**: $0.00" in sec + # The "free tier" clause must mention the routed model verbatim, not the + # stale hardcoded `headroom glm-5.2:cloud` literal. assert "free tier" in sec + assert "glm-5.2:cloud" in sec + # Equivalent should be > 0 for non-trivial token counts. assert "$0.00" in sec # the actual line # Multi-provider table header present, default roster rendered, default # cost_target (Sonnet 5) is the bolded row.