feat(review): multi-lens orchestration — 5 parallel opencode subprocesses (security/docs/code-quality/tests/perf)
On by default, opt-out via "reviewers": []. 290 tests pass.
- pilot/opencode_review.py: ReviewerSpec dataclass, default_reviewers(),
parse_reviewers_config(), parse_triage_config(), resolve_reviewers(),
_normalize_lens_finding(), posthash() (matches feedback.py scheme),
_agreement_hash() (severity-free for cross-lens promotion), _tone_strip(),
synthesize() 7-stage (severity_floor → tone-strip → length cap → per-lens
max → per-file cap → dedup by _posthash → cross-lens severity promote →
per-PR cap), run_lenses() (ThreadPoolExecutor pool=4), triage(),
_intersect_with_triage(), _filter_by_skip_if(), run_lenses_review().
run() routes to fan-out when config.reviewers[] present or PRAGENT_REVIEWERS=1.
- pilot/ai_review.py: parse_repo_config learns reviewers[] and triage objects
(id regex /^[a-z0-9][a-z0-9-]{0,31}$/, 8-entry cap, agent_file/model/
severity_floor/max_findings/activation/skip_if_all_changed_paths/hotpath_globs).
review_pr branches to opencode_review.run_lenses_review when configured.
_render_collapsible_usage shows lenses: ... line when present.
- .opencode/agents/{docs,code-quality,triage}.md: 3 new lens subagents.
- .opencode/skills/lens-orchestration/SKILL.md: strict-JSON contract every
lens subagent MUST honor.
- .opencode/agents/pragent.md: slim to coordinator; no more hardcoded
@security/@tests/@perf delegation; loads lens-orchestration skill.
- .opencode/README.md: rewrite 'Add a review lens' recipe for multi-lens.
- pilot/README-webhook.md: new 'Multi-lens pipeline' section (diagram +
default roster + config schema + env vars + cross-lens dedup contract).
- tests: 36 new tests (test_ai_review.py +12 reviewers/triage/usage,
test_opencode_review.py +24 orchestration). posthash golden-vector matches
feedback.py exactly across 5 severity × 2 line cases.
This commit is contained in:
@@ -1458,3 +1458,138 @@ def test_build_user_prompt_injects_additional_context():
|
||||
def test_build_user_prompt_skips_additional_context_when_empty():
|
||||
prompt = build_user_prompt("T", "B", "diff")
|
||||
assert "## Repo-provided context" not in prompt
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# parse_repo_config: reviewers[] + triage (multi-lens orchestration)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_parse_repo_config_reviewers_array_basic():
|
||||
raw = json.dumps({
|
||||
"reviewers": [
|
||||
{"id": "security", "severity_floor": "high", "max_findings": 10},
|
||||
{"id": "docs", "agent_file": ".opencode/agents/docs.md"},
|
||||
{"id": "perf", "model": "headroom/glm-5.2:cloud",
|
||||
"skip_if_all_changed_paths": "docs/**"},
|
||||
]
|
||||
})
|
||||
cfg = parse_repo_config(raw)
|
||||
assert cfg["reviewers"] == [
|
||||
{"id": "security", "severity_floor": "high", "max_findings": 10},
|
||||
{"id": "docs", "agent_file": ".opencode/agents/docs.md"},
|
||||
{"id": "perf", "model": "headroom/glm-5.2:cloud",
|
||||
"skip_if_all_changed_paths": "docs/**"},
|
||||
]
|
||||
|
||||
|
||||
def test_parse_repo_config_reviewers_rejects_bad_id():
|
||||
# Punctuation, leading dash, underscore, empty — all silently dropped.
|
||||
cfg = parse_repo_config(json.dumps({
|
||||
"reviewers": [
|
||||
{"id": "BAD!!!"},
|
||||
{"id": "-bad-start"},
|
||||
{"id": "ok_under"},
|
||||
{"id": ""},
|
||||
{"id": "good-one"},
|
||||
]
|
||||
}))
|
||||
assert cfg["reviewers"] == [{"id": "good-one"}]
|
||||
|
||||
|
||||
def test_parse_repo_config_reviewers_caps_at_8():
|
||||
cfg = parse_repo_config(json.dumps({
|
||||
"reviewers": [{"id": f"l{i}"} for i in range(12)]
|
||||
}))
|
||||
assert len(cfg["reviewers"]) == 8
|
||||
|
||||
|
||||
def test_parse_repo_config_reviewers_drop_non_dict_entries():
|
||||
cfg = parse_repo_config(json.dumps({
|
||||
"reviewers": ["not-a-dict", 42, None, {"id": "ok"}]
|
||||
}))
|
||||
assert cfg["reviewers"] == [{"id": "ok"}]
|
||||
|
||||
|
||||
def test_parse_repo_config_reviewers_absent_yields_no_key():
|
||||
cfg = parse_repo_config("{}")
|
||||
assert "reviewers" not in cfg
|
||||
|
||||
|
||||
def test_parse_repo_config_reviewers_activation_validated():
|
||||
cfg = parse_repo_config(json.dumps({
|
||||
"reviewers": [
|
||||
{"id": "a", "activation": "auto"},
|
||||
{"id": "b", "activation": "always"},
|
||||
{"id": "c", "activation": "off"},
|
||||
{"id": "d", "activation": "BOGUS"}, # dropped (unsupported)
|
||||
]
|
||||
}))
|
||||
# Only the entries with valid activation carry the key — the BOGUS one
|
||||
# just keeps id (the unknown field is silently dropped, not rejected).
|
||||
assert [r.get("activation") for r in cfg["reviewers"]] == [
|
||||
"auto", "always", "off", None
|
||||
]
|
||||
|
||||
|
||||
def test_parse_repo_config_triage_object_full():
|
||||
cfg = parse_repo_config(json.dumps({
|
||||
"triage": {"enabled": True, "model": "headroom/haiku", "max_lenses": 3}
|
||||
}))
|
||||
assert cfg["triage"] == {"enabled": True, "model": "headroom/haiku", "max_lenses": 3}
|
||||
|
||||
|
||||
def test_parse_repo_config_triage_disabled():
|
||||
cfg = parse_repo_config(json.dumps({"triage": {"enabled": False}}))
|
||||
assert cfg["triage"] == {"enabled": False}
|
||||
|
||||
|
||||
def test_parse_repo_config_triage_malformed_yields_disabled():
|
||||
# Non-object triage value (string, list, number) should disable, not crash.
|
||||
for raw in (
|
||||
'{"triage": "off"}',
|
||||
'{"triage": []}',
|
||||
'{"triage": 42}',
|
||||
):
|
||||
cfg = parse_repo_config(raw)
|
||||
assert cfg.get("triage") == {"enabled": False}, f"failed for {raw}"
|
||||
|
||||
|
||||
def test_parse_repo_config_triage_absent_yields_no_key():
|
||||
cfg = parse_repo_config("{}")
|
||||
assert "triage" not in cfg
|
||||
|
||||
|
||||
def test_parse_repo_config_triage_max_lenses_capped_at_8():
|
||||
cfg = parse_repo_config(json.dumps({"triage": {"max_lenses": 100}}))
|
||||
# 100 is out of range; the key is dropped, not clamped. Caller defaults.
|
||||
assert "max_lenses" not in cfg.get("triage", {})
|
||||
|
||||
|
||||
def test_render_collapsible_usage_shows_lenses_when_multi():
|
||||
usage = {
|
||||
"input": 100, "output": 50, "reasoning": 0,
|
||||
"cache_read": 0, "cache_write": 0, "total": 150,
|
||||
"steps": 12, "duration_s": 8.4,
|
||||
"lenses": ["security", "docs", "tests"],
|
||||
"lens_steps": 12,
|
||||
}
|
||||
out = _render_collapsible_usage(usage, "headroom/glm-5.2:cloud", None)
|
||||
assert "Lenses" in out
|
||||
# All three lens ids are shown in backticks.
|
||||
assert "`security`" in out
|
||||
assert "`docs`" in out
|
||||
assert "`tests`" in out
|
||||
# Step count is surfaced.
|
||||
assert "12" in out
|
||||
|
||||
|
||||
def test_render_collapsible_usage_omits_lenses_when_single_primary():
|
||||
usage = {
|
||||
"input": 100, "output": 50, "reasoning": 0,
|
||||
"cache_read": 0, "cache_write": 0, "total": 150,
|
||||
"steps": 4, "duration_s": 2.0,
|
||||
}
|
||||
out = _render_collapsible_usage(usage, "headroom/glm-5.2:cloud", None)
|
||||
assert "Lenses" not in out
|
||||
|
||||
|
||||
@@ -477,4 +477,298 @@ def test_committed_config_has_no_private_address():
|
||||
cfg = json.loads(open(os.path.join(ROOT, "opencode.json"), encoding="utf-8").read())
|
||||
url = cfg["provider"]["headroom"]["options"]["baseURL"]
|
||||
assert "100." not in url and "192.168." not in url, url
|
||||
assert ".internal" in url or "example" in url, url
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Multi-lens orchestration
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _finding(path="a.ts", line=5, severity="medium", title="bug", body="why",
|
||||
suggestion="fix", rule_id="TST", lens_id="security"):
|
||||
"""Factory: returns a normalized finding (matches _normalize_lens_finding shape)."""
|
||||
return {
|
||||
"severity": severity,
|
||||
"path": path,
|
||||
"line": line,
|
||||
"problem": f"{title}\n\n{body}",
|
||||
"fix": "",
|
||||
"suggestion": suggestion,
|
||||
"reference": "",
|
||||
"_lens": lens_id,
|
||||
"_lens_model": "m1",
|
||||
"_ruleId": rule_id,
|
||||
"_posthash": oc.posthash(path, line, severity, f"{title}\n\n{body}"),
|
||||
}
|
||||
|
||||
|
||||
def test_default_reviewers_returns_five():
|
||||
defaults = oc.default_reviewers()
|
||||
assert len(defaults) == 5
|
||||
ids = [r.id for r in defaults]
|
||||
# Security first (most conservative severity), then docs/code-quality/tests,
|
||||
# then perf (highest severity floor).
|
||||
assert ids[0] == "security"
|
||||
assert "docs" in ids
|
||||
assert "code-quality" in ids
|
||||
assert "tests" in ids
|
||||
assert "perf" in ids
|
||||
# Severity floor is permissive by default; we let apply_repo_config cascade
|
||||
# from style.threshold.
|
||||
assert defaults[0].severity_floor == "low"
|
||||
# Each default resolves to the factory-style agent file path via agent_path().
|
||||
for r in defaults:
|
||||
assert r.agent_file == "" # the default — derived lazily
|
||||
assert r.agent_path("/tmp/fake").endswith(f".opencode/agents/{r.id}.md")
|
||||
|
||||
|
||||
def test_resolve_reviewers_config_overrides_default():
|
||||
cfg = {
|
||||
"reviewers": [
|
||||
{"id": "security", "severity_floor": "high"},
|
||||
{"id": "docs"},
|
||||
]
|
||||
}
|
||||
out = oc.resolve_reviewers(cfg)
|
||||
assert [r.id for r in out] == ["security", "docs"]
|
||||
assert out[0].severity_floor == "high"
|
||||
assert out[1].severity_floor in ("low", "medium") # default fallback
|
||||
|
||||
|
||||
def test_resolve_reviewers_drops_activation_off():
|
||||
cfg = {"reviewers": [
|
||||
{"id": "security"},
|
||||
{"id": "docs", "activation": "off"},
|
||||
{"id": "tests"},
|
||||
]}
|
||||
out = oc.resolve_reviewers(cfg)
|
||||
assert [r.id for r in out] == ["security", "tests"]
|
||||
|
||||
|
||||
def test_resolve_reviewers_falls_back_to_default_when_empty():
|
||||
# Empty array → caller treats as "opt out" but resolve still returns
|
||||
# something concrete; the caller in review_pr must still pass through.
|
||||
out = oc.resolve_reviewers({"reviewers": []})
|
||||
assert [r.id for r in out] == [r.id for r in oc.default_reviewers()]
|
||||
|
||||
|
||||
def test_parse_reviewers_config_rejects_bad_id():
|
||||
bad = oc.parse_reviewers_config([
|
||||
{"id": "BAD!!!"},
|
||||
{"id": "ok"},
|
||||
])
|
||||
assert [r.id for r in bad] == ["ok"]
|
||||
|
||||
|
||||
def test_parse_reviewers_config_caps_at_8():
|
||||
bad = oc.parse_reviewers_config([{"id": f"l{i}"} for i in range(12)])
|
||||
assert len(bad) == 8
|
||||
|
||||
|
||||
def test_synthesize_dedup_by_posthash_keeps_highest_severity():
|
||||
# Same path/line/problem, IDENTICAL severity → posthash collision → 1 survivor.
|
||||
sec = _finding(severity="medium", rule_id="SEC", lens_id="security")
|
||||
tst = _finding(severity="medium", rule_id="TST", lens_id="tests")
|
||||
out = oc.synthesize({"security": [sec], "tests": [tst]},
|
||||
[oc.ReviewerSpec(id="security"),
|
||||
oc.ReviewerSpec(id="tests")],
|
||||
per_file_cap=10)
|
||||
assert len(out) == 1
|
||||
# On a tie, the earlier-listed lens wins (security listed first).
|
||||
assert out[0]["_lens"] == "security"
|
||||
# Multi-lens agreement → one-step promotion: medium → high.
|
||||
assert out[0]["severity"] == "high"
|
||||
assert out[0].get("_multi_lens") is True
|
||||
|
||||
|
||||
def test_synthesize_severity_floor_per_lens():
|
||||
# security with floor=high drops the medium finding before merge.
|
||||
sec = _finding(severity="medium", lens_id="security")
|
||||
out = oc.synthesize({"security": [sec]},
|
||||
[oc.ReviewerSpec(id="security", severity_floor="high")])
|
||||
assert out == []
|
||||
|
||||
|
||||
def test_synthesize_tone_strip():
|
||||
# The opener "Consider" must be stripped from the body.
|
||||
f = _finding(title="Consider using parameterized queries", body="it is safer")
|
||||
out = oc.synthesize({"security": [f]}, [oc.ReviewerSpec(id="security")])
|
||||
assert "Consider" not in out[0]["problem"]
|
||||
assert "parameterized queries" in out[0]["problem"]
|
||||
|
||||
|
||||
def test_synthesize_per_file_cap_drops_lowest_severity():
|
||||
fs = [
|
||||
_finding(line=1, severity="low"),
|
||||
_finding(line=2, severity="medium"),
|
||||
_finding(line=3, severity="high"),
|
||||
]
|
||||
out = oc.synthesize({"security": fs}, [oc.ReviewerSpec(id="security")],
|
||||
per_file_cap=2)
|
||||
assert len(out) == 2
|
||||
# The low-severity one was dropped (lowest).
|
||||
assert all(f["severity"] != "low" for f in out)
|
||||
|
||||
|
||||
def test_synthesize_per_pr_cap():
|
||||
fs = [
|
||||
_finding(line=1, severity="high"),
|
||||
_finding(line=2, severity="medium"),
|
||||
_finding(line=3, severity="low"),
|
||||
]
|
||||
out = oc.synthesize({"security": fs}, [oc.ReviewerSpec(id="security")],
|
||||
per_pr_cap=2)
|
||||
assert len(out) == 2
|
||||
# Highest severity first.
|
||||
assert out[0]["severity"] == "high"
|
||||
|
||||
|
||||
def test_synthesize_cross_lens_promotion_and_multi_tag():
|
||||
# Severity-keyed posthash differs, so the agreement_hash (severity-free)
|
||||
# collapses them at the multi-lens stage, surviving separately but
|
||||
# promoted + tagged.
|
||||
sec = _finding(severity="medium", lens_id="security")
|
||||
tst = _finding(severity="high", lens_id="tests")
|
||||
out = oc.synthesize({"security": [sec], "tests": [tst]},
|
||||
[oc.ReviewerSpec(id="security"),
|
||||
oc.ReviewerSpec(id="tests")])
|
||||
assert len(out) == 2
|
||||
# Both got _multi_lens tag.
|
||||
assert all(f.get("_multi_lens") is True for f in out)
|
||||
# Both got a one-step promotion.
|
||||
sev_rank = oc.SEVERITY_RANK
|
||||
for f in out:
|
||||
if f["_lens"] == "security":
|
||||
assert f["severity"] == "high" # medium → high
|
||||
else:
|
||||
assert f["severity"] == "critical" # high → critical
|
||||
|
||||
|
||||
def test_synthesize_promotion_never_past_critical():
|
||||
# A critical finding stays critical even with multi-lens confirmation.
|
||||
f = _finding(severity="critical", lens_id="security")
|
||||
other = _finding(severity="critical", lens_id="tests")
|
||||
out = oc.synthesize({"security": [f], "tests": [other]},
|
||||
[oc.ReviewerSpec(id="security"),
|
||||
oc.ReviewerSpec(id="tests")])
|
||||
# Both critical → both tagged, neither promoted past critical.
|
||||
assert all(f["severity"] == "critical" for f in out)
|
||||
assert all(f.get("_multi_lens") is True for f in out)
|
||||
|
||||
|
||||
def test_synthesize_caps_lens_max_findings():
|
||||
# 20 medium findings on DIFFERENT files (so per_file_cap doesn't kick in).
|
||||
fs = [_finding(path=f"a{i}.ts", line=i + 1, severity="medium") for i in range(20)]
|
||||
out = oc.synthesize(
|
||||
{"security": fs}, [oc.ReviewerSpec(id="security", max_findings=5)],
|
||||
per_file_cap=10,
|
||||
)
|
||||
assert len(out) == 5
|
||||
|
||||
|
||||
def test_synthesize_returns_empty_on_empty_input():
|
||||
assert oc.synthesize({}, []) == []
|
||||
assert oc.synthesize({"security": []}, [oc.ReviewerSpec(id="security")]) == []
|
||||
|
||||
|
||||
def test_normalize_lens_finding_rejects_bad_inputs():
|
||||
spec = oc.ReviewerSpec(id="security")
|
||||
# Missing path
|
||||
assert oc._normalize_lens_finding(
|
||||
{"line": 1, "severity": "low", "title": "x", "body": "y"}, spec, "m"
|
||||
) is None
|
||||
# Non-int line
|
||||
assert oc._normalize_lens_finding(
|
||||
{"path": "a.ts", "line": "abc", "severity": "low", "title": "x", "body": "y"}, spec, "m"
|
||||
) is None
|
||||
# Line 0
|
||||
assert oc._normalize_lens_finding(
|
||||
{"path": "a.ts", "line": 0, "severity": "low", "title": "x", "body": "y"}, spec, "m"
|
||||
) is None
|
||||
# Empty title+body
|
||||
assert oc._normalize_lens_finding(
|
||||
{"path": "a.ts", "line": 1, "severity": "low", "title": "", "body": ""}, spec, "m"
|
||||
) is None
|
||||
# Unknown severity → coerced to medium
|
||||
out = oc._normalize_lens_finding(
|
||||
{"path": "a.ts", "line": 1, "severity": "URGENT", "title": "x", "body": "y"}, spec, "m"
|
||||
)
|
||||
assert out["severity"] == "medium"
|
||||
|
||||
|
||||
def test_posthash_matches_feedback_posthash():
|
||||
# Golden vector: identical inputs must produce identical 16-char hex.
|
||||
import feedback as fb
|
||||
cases = [
|
||||
("a/b.ts", 12, "critical", "SQL injection via string concat"),
|
||||
("a/b.ts", 12, "medium", "SQL injection via string concat"),
|
||||
("other.py", 99, "low", "docstring out of sync"),
|
||||
("", 0, "info", "empty"),
|
||||
]
|
||||
for path, line, sev, problem in cases:
|
||||
ours = oc.posthash(path, line, sev, problem)
|
||||
theirs = fb.posthash(path, line, sev, problem)
|
||||
assert ours == theirs, (
|
||||
f"posthash drift: path={path} line={line} sev={sev} "
|
||||
f"ours={ours} feedback={theirs}"
|
||||
)
|
||||
|
||||
|
||||
def test_extract_json_object_tolerates_fences_and_prose():
|
||||
# Plain JSON
|
||||
assert oc._extract_json_object('{"a":1}') == {"a": 1}
|
||||
# Mixed with prose
|
||||
assert oc._extract_json_object('hello\n{"a":2}\nbye') == {"a": 2}
|
||||
# Fenced (last one wins)
|
||||
text = 'first\n```json\n{"a":1}\n```\nthen\n```json\n{"a":2}\n```\n'
|
||||
assert oc._extract_json_object(text) == {"a": 2}
|
||||
# Malformed
|
||||
assert oc._extract_json_object("not json at all") is None
|
||||
assert oc._extract_json_object("") is None
|
||||
|
||||
|
||||
def test_filter_by_skip_if_all_changed_paths():
|
||||
reviewers = [
|
||||
oc.ReviewerSpec(id="docs", skip_if_all_changed_paths="**/*.md"),
|
||||
oc.ReviewerSpec(id="security"),
|
||||
]
|
||||
# All changed paths are .md → docs skipped.
|
||||
out = oc._filter_by_skip_if(reviewers, ["docs/a.md", "docs/b.md"])
|
||||
assert [r.id for r in out] == ["security"]
|
||||
# Mixed paths → docs not skipped.
|
||||
out = oc._filter_by_skip_if(reviewers, ["docs/a.md", "src/main.py"])
|
||||
assert [r.id for r in out] == ["docs", "security"]
|
||||
|
||||
|
||||
def test_intersect_with_triage_preserves_order():
|
||||
reviewers = [
|
||||
oc.ReviewerSpec(id="security"),
|
||||
oc.ReviewerSpec(id="docs"),
|
||||
oc.ReviewerSpec(id="tests"),
|
||||
]
|
||||
out = oc._intersect_with_triage(reviewers, ["docs", "security"])
|
||||
assert [r.id for r in out] == ["security", "docs"]
|
||||
|
||||
|
||||
def test_intersect_with_triage_none_returns_all():
|
||||
reviewers = [oc.ReviewerSpec(id="security"), oc.ReviewerSpec(id="docs")]
|
||||
assert oc._intersect_with_triage(reviewers, None) == reviewers
|
||||
assert oc._intersect_with_triage(reviewers, []) == reviewers
|
||||
|
||||
|
||||
def test_merge_usage_sums_tokens():
|
||||
a = {"input": 100, "output": 50, "cache_read": 10, "cache_write": 5, "steps": 3}
|
||||
b = {"input": 200, "output": 80, "cache_read": 0, "cache_write": 4, "steps": 4}
|
||||
merged = oc.merge_usage([a, b])
|
||||
assert merged["input"] == 300
|
||||
assert merged["output"] == 130
|
||||
assert merged["cache_read"] == 10
|
||||
assert merged["cache_write"] == 9
|
||||
assert merged["steps"] == 7
|
||||
|
||||
|
||||
def test_merge_usage_skips_none():
|
||||
a = {"input": 100, "output": 50, "steps": 3}
|
||||
merged = oc.merge_usage([a, None, None])
|
||||
assert merged["input"] == 100
|
||||
assert merged["steps"] == 3
|
||||
|
||||
Reference in New Issue
Block a user