From d0f99b97635bba6c058f924bf767c69ab621b0a0 Mon Sep 17 00:00:00 2001 From: Marcos Date: Sat, 22 Aug 2026 14:46:47 +0000 Subject: [PATCH] feat(ai_review): parse .pr-review.json:model as per-repo override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repos that need to pin the review engine (e.g. 'this project requires claude-sonnet-5 for the budget line' or 'route everything through gpt-5.6-luna for now') can declare a top-level 'model' string in .pr-review.json. The parser validates the value against cost_model.PRICES (lazy import — ollama path stays dep-free) and silently drops unknown values with a stderr pointer to the valid key set so a typo in the config file surfaces in the logs instead of silently falling back. The orchestrator gains a small _resolve_display_model(base, config) helper that implements a 3-way precedence: 1. OPENCODE_MODEL env (operator override, used verbatim) 2. config['model'] (per-repo override) 3. f'headroom/{base}' (default) review_pr resolves once (lazy fallback before config is loaded) and re-resolves after .pr-review.json is fetched, then threads the result into the opencode subprocess, REVIEW_HEADER, and the cost-line parenthetical. Same single value everywhere — no more mix of bare OLLAMA_MODEL id in the header and a stale free-tier literal in the cost line. Tests cover parsing acceptance, parsing rejection (capsys stderr), type validation, precedence in all 4 (env×config) combinations, and an end-to-end sanity check that format_review_body shows the override and not the base id. --- pilot/ai_review.py | 65 +++++++++++++++++++++++++++++++---- tests/pilot/test_ai_review.py | 56 ++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 6 deletions(-) diff --git a/pilot/ai_review.py b/pilot/ai_review.py index 5c754c9..f03ae7f 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -374,6 +374,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. @@ -1169,6 +1195,7 @@ def parse_repo_config(raw: str) -> dict: 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 """ @@ -1236,6 +1263,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] = [] @@ -1858,12 +1905,12 @@ def review_pr( Both the CI `run()` entry point and the central webhook server call this. """ try: - # Resolve the model display name once and pass it to every consumer - # (opencode subprocess, REVIEW_HEADER, cost-line parenthetical). Env - # override wins; otherwise we prefix the OLLAMA_MODEL bare id with - # the headroom provider so the line reads `headroom/` instead - # of a stale hardcoded literal. - display_model = os.environ.get("OPENCODE_MODEL") or f"headroom/{model}" + # 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. @@ -1879,6 +1926,12 @@ def review_pr( 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 diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index 859ef8d..aa78a8e 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -419,6 +419,62 @@ def test_parse_repo_config_static_message_ignores_blank(): 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 # ---------------------------------------------------------------------------