Merge agent-B: cost-line fix + static_message + per-repo model override

# Conflicts:
#	pilot/ai_review.py
#	tests/pilot/test_ai_review.py
This commit is contained in:
Marcos
2026-08-22 15:01:38 +00:00
2 changed files with 223 additions and 12 deletions
+95 -12
View File
@@ -273,12 +273,18 @@ def format_review_body(
walkthrough: list[str] | None = None, walkthrough: list[str] | None = None,
risk_verdict: str = "", risk_verdict: str = "",
test_coverage: str = "", test_coverage: str = "",
static_message: str = "",
) -> str: ) -> str:
"""Format the posted review summary body. """Format the posted review summary body.
Layout (per the operator's format guide): Layout (per the operator's format guide):
* Header line (``🤖 AI Review …``) including the merge-confidence badge. * 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** — 24 bullets of what the PR introduces * **Summary of Changes** — 24 bullets of what the PR introduces
(`summary_changes`); falls back to the opencode prose `summary` if (`summary_changes`); falls back to the opencode prose `summary` if
the agent didn't emit the list. the agent didn't emit the list.
@@ -317,6 +323,11 @@ def format_review_body(
) )
parts: list[str] = [header] 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 --- # --- Summary of Changes ---
sc = list(summary_changes or []) sc = list(summary_changes or [])
if not sc and summary: if not sc and summary:
@@ -467,6 +478,32 @@ def _resolve_price_target(config: dict | None) -> tuple[str, str | None]:
return chosen, 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: def equivalent_cost(usage: dict, price_key: str) -> float:
"""USD the measured usage would have billed on `price_key`'s provider. """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`, would bill on mainstream paid APIs (configurable via `compare_against`,
defaulting to ``DEFAULT_COMPARE_AGAINST``). The row matching `cost_target` defaulting to ``DEFAULT_COMPARE_AGAINST``). The row matching `cost_target`
is bolded so the price target stands out. The whole table is omitted when 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: if not usage:
return "" 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 "?" dur_s = f"{dur}s" if dur is not None else "?"
actual = usage.get("cost") or 0.0 actual = usage.get("cost") or 0.0
actual_s = f"${actual:.4f}" if actual else "$0.00" 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) cost_target, price_err = _resolve_price_target(config)
if price_err: if price_err:
# Surface config typos loudly but do not pollute the posted summary # 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}" cost_str = f"${c:.4f}" if c < 0.01 else f"${c:.2f}"
bold = "**" if key == cost_target else "" bold = "**" if key == cost_target else ""
eq_rows.append(f"| {bold}{label}{bold} | {cost_str} |") eq_rows.append(f"| {bold}{label}{bold} | {cost_str} |")
in_tok = usage.get("input", 0) in_tok = usage.get("input", 0)
out_tok = usage.get("output", 0) out_tok = usage.get("output", 0)
reason_tok = usage.get("reasoning", 0) reason_tok = usage.get("reasoning", 0)
@@ -1288,6 +1330,7 @@ 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_PATTERNS_ITEMS = 16 # allow + deny separately, total 32 entries
CONFIG_MAX_FINDINGS = 30 CONFIG_MAX_FINDINGS = 30
CONFIG_MAX_STATIC_MESSAGE_CHARS = 400 # free-text banner, mirror of instructions
STYLES = frozenset(STYLE_DEFAULTS) STYLES = frozenset(STYLE_DEFAULTS)
SEVERITY_VALUES = frozenset(SEVERITIES) SEVERITY_VALUES = frozenset(SEVERITIES)
@@ -1303,12 +1346,14 @@ def parse_repo_config(raw: str) -> dict:
Recognised keys (all optional): Recognised keys (all optional):
focus, exclude_paths, languages, instructions — text steer focus, exclude_paths, languages, instructions — text steer
static_message ≤ CONFIG_MAX_STATIC_MESSAGE_CHARS — banner under header
style strict|balanced|lenient — default: balanced style strict|balanced|lenient — default: balanced
severity_threshold low|medium|high|critical — default: per style severity_threshold low|medium|high|critical — default: per style
max_findings 1..CONFIG_MAX_FINDINGS — default: per style max_findings 1..CONFIG_MAX_FINDINGS — default: per style
exclude_tests bool — default: False exclude_tests bool — default: False
require_tests bool — default: False require_tests bool — default: False
patterns {allow:[…], deny:[…]} — post-filter globs patterns {allow:[…], deny:[…]} — post-filter globs
model <key of cost_model.PRICES> — per-repo override
cost_target <key of cost_model.PRICES> — see equivalent_cost cost_target <key of cost_model.PRICES> — see equivalent_cost
additional_context_urls list[str] (≤ 8) — see fetch_additional_context 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(): if isinstance(instr, str) and instr.strip():
out["instructions"] = instr.strip()[:CONFIG_MAX_INSTRUCTIONS_CHARS] 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") style = data.get("style")
if isinstance(style, str) and style.strip().lower() in STYLES: if isinstance(style, str) and style.strip().lower() in STYLES:
out["style"] = style.strip().lower() out["style"] = style.strip().lower()
@@ -1372,6 +1421,26 @@ def parse_repo_config(raw: str) -> dict:
if isinstance(ct, str) and ct.strip(): if isinstance(ct, str) and ct.strip():
out["cost_target"] = 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") acu = data.get("additional_context_urls")
if isinstance(acu, list): if isinstance(acu, list):
urls: list[str] = [] urls: list[str] = []
@@ -2023,6 +2092,13 @@ def review_pr(
Both the CI `run()` entry point and the central webhook server call this. Both the CI `run()` entry point and the central webhook server call this.
""" """
try: 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) reviews = fetch_existing_reviews(api, repo, index, token)
# Dedupe: already reviewed this exact commit -> nothing to do. # Dedupe: already reviewed this exact commit -> nothing to do.
if sha and sha in reviewed_shas(reviews): 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) raw_diff, _truncated, _orig = fetch_pr_diff(api, repo, index, token, max_chars)
if not raw_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.", display_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 = compact_prior_reviews(prior_review_bodies(reviews, sha)) 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 # Trim the diff to +/- hunks plus a narrow context window. The agent
# resends the brief prefix every step, so a 25k-char diff becomes # resends the brief prefix every step, so a 25k-char diff becomes
# 25k × 30-step × cached-after-step-1 = hundreds of thousands of input # 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 # the brief, and the pragent agent factory; returns stdout with a
# summary + findings JSON. We parse + anchor + post here. # summary + findings JSON. We parse + anchor + post here.
import opencode_review # local import keeps the ollama path dep-free import opencode_review # local import keeps the ollama path dep-free
# opencode wants a provider-prefixed model ref (headroom/glm-5.2:cloud); # Reuse the display_model resolved above for the subprocess — same
# `model` here is the bare id (OLLAMA_MODEL). OPENCODE_MODEL overrides # provider-prefixed ref goes to the engine and into the review body.
# with the full ref; otherwise we prefix the configured provider. oc_model = display_model
oc_model = os.environ.get("OPENCODE_MODEL") or f"headroom/{model}"
# Multi-lens fan-out: when the repo declared `reviewers[]` (or the # Multi-lens fan-out: when the repo declared `reviewers[]` (or the
# operator pinned PRAGENT_REVIEWERS=1), spawn one opencode subprocess # operator pinned PRAGENT_REVIEWERS=1), spawn one opencode subprocess
# per lens in parallel and synthesize. Falls through to the legacy # per lens in parallel and synthesize. Falls through to the legacy
@@ -2109,10 +2190,11 @@ def review_pr(
file=sys.stderr, flush=True, file=sys.stderr, flush=True,
) )
salvaged = salvage_summary(stdout) 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( 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)) display_model, sha, usage_section=usage_section,
static_message=(config or {}).get("static_message", "")))
return True return True
else: else:
user_prompt = build_user_prompt(title, body + compression_note, diff, config, prior, additional_context) 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. # estimates. Only meaningful when we have measured usage.
if usage and usage.get("output"): if usage and usage.get("output"):
compute_attribution(findings, usage["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 # Anchor against the RAW diff, never the compressed one. Compression
# drops context lines, so a finding on a line that survived in the file # drops context lines, so a finding on a line that survived in the file
@@ -2184,7 +2266,7 @@ def review_pr(
# reach this call. # reach this call.
confidence = merge_confidence(findings, multi_lens_observed=multi_lens) confidence = merge_confidence(findings, multi_lens_observed=multi_lens)
summary_body = format_review_body( summary_body = format_review_body(
"\n\n".join(summary_parts), model, sha, "\n\n".join(summary_parts), display_model, sha,
summary=review_summary, summary=review_summary,
usage_section=usage_section, usage_section=usage_section,
summary_changes=summary_changes, summary_changes=summary_changes,
@@ -2192,6 +2274,7 @@ def review_pr(
findings_for_table=findings, findings_for_table=findings,
inline_count=len(anchored), inline_count=len(anchored),
confidence=confidence, confidence=confidence,
static_message=(config or {}).get("static_message", ""),
) )
post_inline_review(api, repo, index, token, summary_body, anchored) post_inline_review(api, repo, index, token, summary_body, anchored)
@@ -2203,7 +2286,7 @@ def review_pr(
return True return True
except Exception as e: # fail-open except Exception as e: # fail-open
try: 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: except Exception as e2:
print(f"pragent: could not post failure note: {e2}", file=sys.stderr) print(f"pragent: could not post failure note: {e2}", file=sys.stderr)
print(f"pragent: review failed: {e}", file=sys.stderr) print(f"pragent: review failed: {e}", file=sys.stderr)
+128
View File
@@ -410,6 +410,81 @@ def test_parse_repo_config_partial_and_bad():
assert parse_repo_config('{"instructions":" "}') == {"enabled": False} 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/<base>
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 # 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} "cache_write": 0, "total": 10, "cost": 0.0123, "steps": 1, "duration_s": 1.0}
sec = _render_collapsible_usage(usage, "m", config=None) sec = _render_collapsible_usage(usage, "m", config=None)
assert "$0.0123" in sec 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 "(<model> — 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 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 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(): def test_format_review_body_with_summary_changes_and_risks():
body = format_review_body( body = format_review_body(
"", "glm-5.2:cloud", "abcdef1234567890", "", "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). # grok-4.5; cost_target defaults to sonnet-5 (bolded).
assert "🔋 AI Usage & Run Details" in sec assert "🔋 AI Usage & Run Details" in sec
assert "**Actual**: $0.00" 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 "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 assert "$0.00" in sec # the actual line
# Multi-provider table header present, default roster rendered, default # Multi-provider table header present, default roster rendered, default
# cost_target (Sonnet 5) is the bolded row. # cost_target (Sonnet 5) is the bolded row.