feat(ai_review): parse .pr-review.json:model as per-repo override
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.
This commit is contained in:
@@ -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/<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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user