feat: add unified-review workflow (scoring tools + skill)
Adds the unified-review integration that fuses CRG graph context with the ai-code-review scoring methodology and gstack-review fix-first workflow: - scoring.py: objective Layer-2 metrics (sql_risk, exception_coverage, redundancy_rate, high_risk_density, vulnerability_risk) with good/warn/fail grades, plus dedupe_findings (fingerprint merge, multi-source confidence boost, PR quality score) and report data builder - tools/scoring_tools.py + main.py: three new MCP tools (score_review_tool, dedupe_findings_tool, generate_report_tool) - assets/report-template.html: self-contained HTML report template - skills.py + skills/unified-review/: new read-only unified-review skill with language/manual-review/specialist checklists - docs and CHANGELOG updated; tests added (test_scoring, test_report, test_unified_review) and test_skills updated for 5 skills
This commit is contained in:
dev
@@ -0,0 +1,105 @@
|
||||
"""Tests for the unified-review HTML report tool."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent))
|
||||
|
||||
from code_review_graph.scoring import build_report_data # noqa: E402
|
||||
from code_review_graph.tools.scoring_tools import ( # noqa: E402
|
||||
_load_report_template,
|
||||
generate_report_func,
|
||||
)
|
||||
|
||||
|
||||
def _review_data() -> dict:
|
||||
return {
|
||||
"scope": "change-level",
|
||||
"tier": "standard",
|
||||
"timestamp": "2026-08-05T00:00:00Z",
|
||||
"files": "app.py, main.py",
|
||||
"baseline": "generic",
|
||||
"verdict": "\u2705 PASS",
|
||||
"metrics": {
|
||||
"sql_risk": {"value": 0, "grade": "good", "note": "heuristic"},
|
||||
"exception_coverage": {"value": 33.33, "grade": "warn", "note": "heuristic"},
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"severity": "MAJOR",
|
||||
"category": "Data",
|
||||
"message": "Possible N+1 query",
|
||||
"path": "src/app.py",
|
||||
"line": 42,
|
||||
"confidence": 7,
|
||||
"fix": "Add eager loading",
|
||||
},
|
||||
],
|
||||
"manual_review": ["Payment callback idempotency"],
|
||||
"llm_judged": ["requirement_coverage", "logic_alignment"],
|
||||
}
|
||||
|
||||
|
||||
class TestBuildReportData:
|
||||
def test_normalises_review_data(self):
|
||||
data = build_report_data(_review_data())
|
||||
assert data["verdict"] == "\u2705 PASS"
|
||||
assert data["tier"] == "standard"
|
||||
assert data["scope"] == "change-level"
|
||||
assert data["metrics"]["sql_risk"]["grade"] == "good"
|
||||
assert len(data["issues"]) == 1
|
||||
assert data["issues"][0]["location"] == "src/app.py:42"
|
||||
assert "requirement_coverage" in data["llm_judged"]
|
||||
|
||||
def test_empty_findings(self):
|
||||
rd = _review_data()
|
||||
rd["findings"] = []
|
||||
data = build_report_data(rd)
|
||||
assert data["issues"] == []
|
||||
|
||||
def test_defaults(self):
|
||||
data = build_report_data({})
|
||||
assert data["scope"] == "change-level"
|
||||
assert data["tier"] == "standard"
|
||||
assert data["verdict"] == "\u274c FAIL"
|
||||
assert data["issues"] == []
|
||||
|
||||
|
||||
class TestLoadTemplate:
|
||||
def test_loads_package_asset(self):
|
||||
template = _load_report_template()
|
||||
assert "{{REPORT_DATA}}" in template
|
||||
assert template.startswith("<!DOCTYPE html>")
|
||||
|
||||
|
||||
class TestGenerateReport:
|
||||
def test_writes_self_contained_html(self, tmp_path):
|
||||
Path(tmp_path, ".code-review-graph").mkdir(exist_ok=True)
|
||||
out = tmp_path / "sub" / "report.html"
|
||||
result = generate_report_func(
|
||||
_review_data(),
|
||||
output_path=str(out),
|
||||
repo_root=str(tmp_path),
|
||||
)
|
||||
assert result["status"] == "ok"
|
||||
assert Path(result["output_path"]).is_file()
|
||||
html = Path(result["output_path"]).read_text(encoding="utf-8")
|
||||
assert "{{REPORT_DATA}}" not in html
|
||||
assert "\u2705 PASS" in html
|
||||
assert "N+1" in html
|
||||
|
||||
def test_default_output_path_is_repo_root(self, tmp_path):
|
||||
# repo_root must look like a project root: create .code-review-graph.
|
||||
Path(tmp_path, ".code-review-graph").mkdir(exist_ok=True)
|
||||
result = generate_report_func(_review_data(), repo_root=str(tmp_path))
|
||||
assert result["status"] == "ok"
|
||||
assert result["output_path"].endswith("code-review-report.html")
|
||||
assert Path(result["output_path"]).is_file()
|
||||
|
||||
def test_handles_missing_repo_root(self, tmp_path):
|
||||
with pytest.raises(ValueError):
|
||||
generate_report_func(_review_data(), repo_root=str(tmp_path / "missing"))
|
||||
@@ -0,0 +1,223 @@
|
||||
"""Tests for the unified-review scoring module (score_review metrics)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent))
|
||||
|
||||
from code_review_graph.scoring import ( # noqa: E402
|
||||
THRESHOLDS,
|
||||
_grade,
|
||||
compute_exception_coverage,
|
||||
compute_redundancy_rate,
|
||||
compute_sql_risk,
|
||||
compute_vulnerability_heuristic,
|
||||
dedupe_findings,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tmp_repo(tmp_path: Path) -> Path:
|
||||
"""A small repository root with one source file."""
|
||||
Path(tmp_path, "app.py").write_text(
|
||||
'import sqlite3\n'
|
||||
'def query(conn, user_id):\n'
|
||||
' conn.execute("SELECT * FROM users WHERE id = " + str(user_id))\n'
|
||||
'def safe(conn, user_id):\n'
|
||||
' conn.execute("SELECT * FROM users WHERE id = ?", (user_id,))\n'
|
||||
'def risky(a):\n'
|
||||
' try:\n'
|
||||
' return compute(a)\n'
|
||||
' except Exception as e:\n'
|
||||
' return None\n',
|
||||
encoding="utf-8",
|
||||
)
|
||||
return tmp_path
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _grade threshold boundaries
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestGradeThresholds:
|
||||
def test_sql_risk_counts(self):
|
||||
# 0 = good, warn_at count = warn, fail_at count = fail.
|
||||
assert _grade("sql_risk", 0) == "good"
|
||||
assert _grade("sql_risk", 1) == "warn"
|
||||
assert _grade("sql_risk", 2) == "warn"
|
||||
assert _grade("sql_risk", 3) == "fail"
|
||||
|
||||
def test_vulnerability_risk_counts(self):
|
||||
assert _grade("vulnerability_risk", 0) == "good"
|
||||
assert _grade("vulnerability_risk", 1) == "warn"
|
||||
assert _grade("vulnerability_risk", 2) == "fail"
|
||||
|
||||
def test_exception_coverage_pct(self):
|
||||
assert _grade("exception_coverage", 60.0) == "good"
|
||||
assert _grade("exception_coverage", 40.0) == "warn"
|
||||
assert _grade("exception_coverage", 20.0) == "fail"
|
||||
|
||||
def test_redundancy_rate_pct(self):
|
||||
assert _grade("redundancy_rate", 5.0) == "good"
|
||||
assert _grade("redundancy_rate", 15.0) == "warn"
|
||||
assert _grade("redundancy_rate", 25.0) == "fail"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Metric functions
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestSQLRisk:
|
||||
def test_detects_interpolated_sql(self, tmp_repo):
|
||||
result = compute_sql_risk(["app.py"], tmp_repo)
|
||||
assert result["value"] >= 1
|
||||
assert result["grade"] in ("warn", "fail")
|
||||
assert any("line" in loc for loc in result["evidence"])
|
||||
|
||||
def test_clean_file_no_risk(self, tmp_path):
|
||||
Path(tmp_path, "clean.py").write_text(
|
||||
"def f(a):\n return a * 2\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
result = compute_sql_risk(["clean.py"], tmp_path)
|
||||
assert result["value"] == 0
|
||||
assert result["grade"] == "good"
|
||||
|
||||
|
||||
class TestExceptionCoverage:
|
||||
def test_counts_exception_paths(self, tmp_repo):
|
||||
result = compute_exception_coverage(["app.py"], tmp_repo)
|
||||
# app.py has a try/except path, so exception coverage > 0.
|
||||
assert result["value"] > 0
|
||||
assert "exception_path_lines" in result["evidence"]
|
||||
|
||||
def test_empty_file(self, tmp_path):
|
||||
Path(tmp_path, "e.py").write_text("# comment only\n", encoding="utf-8")
|
||||
result = compute_exception_coverage(["e.py"], tmp_path)
|
||||
assert result["value"] == 0.0
|
||||
|
||||
|
||||
class TestRedundancy:
|
||||
def test_repeated_blocks_detected(self, tmp_path):
|
||||
body = (
|
||||
"def transform_item(item):\n"
|
||||
" return item.strip().lower().replace(' ', '_')\n"
|
||||
)
|
||||
Path(tmp_path, "r.py").write_text(
|
||||
body + body + body + "def other(x):\n return x\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
result = compute_redundancy_rate(["r.py"], tmp_path)
|
||||
# The shared normalised transform line appears >=3 times.
|
||||
assert result["value"] > 0
|
||||
assert len(result["evidence"]) >= 1
|
||||
|
||||
def test_no_redundancy(self, tmp_path):
|
||||
Path(tmp_path, "n.py").write_text(
|
||||
"def a(x):\n return x\n"
|
||||
"def b(y):\n return y * 2\n"
|
||||
"def c(z):\n return z - 1\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
result = compute_redundancy_rate(["n.py"], tmp_path)
|
||||
assert result["value"] == 0.0
|
||||
|
||||
|
||||
class TestVulnerabilityHeuristic:
|
||||
def test_detects_secret_like_pattern(self, tmp_path):
|
||||
Path(tmp_path, "s.py").write_text(
|
||||
'password = "hunter2"\ndef f():\n pass\n',
|
||||
encoding="utf-8",
|
||||
)
|
||||
result = compute_vulnerability_heuristic(["s.py"], tmp_path)
|
||||
assert result["value"] >= 1
|
||||
assert result["grade"] in ("warn", "fail")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# dedupe_findings
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestDedupeFindings:
|
||||
def test_same_fingerprint_merges(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 8, "source": "main"},
|
||||
{"path": "a.py", "line": 1, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 6, "source": "security"},
|
||||
]
|
||||
result = dedupe_findings(findings)
|
||||
assert len(result["merged"]) == 1
|
||||
# Multi-source confirmed: confidence 8 -> 9 (cap 10).
|
||||
assert result["merged"][0]["confidence"] == 9.0
|
||||
assert result["merged"][0]["multi_source_confirmed"] is True
|
||||
|
||||
def test_confidence_cap_at_10(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 10, "source": "main"},
|
||||
{"path": "a.py", "line": 1, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 10, "source": "security"},
|
||||
]
|
||||
result = dedupe_findings(findings)
|
||||
assert result["merged"][0]["confidence"] == 10.0
|
||||
|
||||
def test_low_confidence_suppressed(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "style",
|
||||
"severity": "INFORMATIONAL", "confidence": 1, "source": "main"},
|
||||
]
|
||||
result = dedupe_findings(findings)
|
||||
assert result["merged"] == []
|
||||
assert result["suppressed_by_confidence"] == 1
|
||||
|
||||
def test_appendix_routing(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "style",
|
||||
"severity": "INFORMATIONAL", "confidence": 3, "source": "main"},
|
||||
]
|
||||
result = dedupe_findings(findings)
|
||||
assert result["merged"][0]["display"] == "appendix"
|
||||
|
||||
def test_quality_score_formula(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 9, "source": "main"},
|
||||
{"path": "b.py", "line": 2, "category": "style",
|
||||
"severity": "INFORMATIONAL", "confidence": 7, "source": "main"},
|
||||
{"path": "c.py", "line": 3, "category": "style",
|
||||
"severity": "INFORMATIONAL", "confidence": 7, "source": "main"},
|
||||
]
|
||||
result = dedupe_findings(findings)
|
||||
# 10 - (1*2 + 2*0.5) = 10 - 3 = 7.0
|
||||
assert result["quality_score"] == 7.0
|
||||
assert result["counts"] == {"critical": 1, "informational": 2}
|
||||
|
||||
def test_prior_suppression(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "style",
|
||||
"severity": "INFORMATIONAL", "confidence": 8, "source": "main"},
|
||||
]
|
||||
prior = [
|
||||
{"path": "a.py", "line": 1, "category": "style"},
|
||||
]
|
||||
result = dedupe_findings(findings, suppress_prior=prior)
|
||||
assert result["merged"] == []
|
||||
assert result["suppressed_by_prior"] == 1
|
||||
|
||||
def test_distinct_fingerprints_not_merged(self):
|
||||
findings = [
|
||||
{"path": "a.py", "line": 1, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 8, "source": "main"},
|
||||
{"path": "a.py", "line": 2, "category": "sql",
|
||||
"severity": "CRITICAL", "confidence": 8, "source": "main"},
|
||||
]
|
||||
result = dedupe_findings(findings)
|
||||
assert len(result["merged"]) == 2
|
||||
@@ -118,7 +118,7 @@ class TestGenerateSkills:
|
||||
assert result.is_dir()
|
||||
assert result == tmp_path / ".claude" / "skills"
|
||||
|
||||
def test_creates_four_skill_subdirs(self, tmp_path):
|
||||
def test_creates_five_skill_subdirs(self, tmp_path):
|
||||
skills_dir = generate_skills(tmp_path)
|
||||
subdirs = sorted(f.name for f in skills_dir.iterdir() if f.is_dir())
|
||||
assert subdirs == [
|
||||
@@ -126,6 +126,7 @@ class TestGenerateSkills:
|
||||
"explore-codebase",
|
||||
"refactor-safely",
|
||||
"review-changes",
|
||||
"unified-review",
|
||||
]
|
||||
for d in skills_dir.iterdir():
|
||||
assert (d / "SKILL.md").is_file()
|
||||
@@ -154,6 +155,7 @@ class TestGenerateSkills:
|
||||
"explore-codebase",
|
||||
"refactor-safely",
|
||||
"review-changes",
|
||||
"unified-review",
|
||||
):
|
||||
for skill_file in (
|
||||
generated / skill_name / "SKILL.md",
|
||||
@@ -167,7 +169,7 @@ class TestGenerateSkills:
|
||||
result = generate_skills(tmp_path, skills_dir=custom)
|
||||
assert result == custom
|
||||
assert result.is_dir()
|
||||
assert len(list(result.iterdir())) == 4
|
||||
assert len(list(result.iterdir())) == 5
|
||||
|
||||
def test_skill_content_includes_get_minimal_context(self, tmp_path):
|
||||
"""Every skill template must reference get_minimal_context."""
|
||||
@@ -192,7 +194,7 @@ class TestGenerateSkills:
|
||||
generate_skills(tmp_path)
|
||||
generate_skills(tmp_path)
|
||||
skills_dir = tmp_path / ".claude" / "skills"
|
||||
assert len(list(skills_dir.iterdir())) == 4
|
||||
assert len(list(skills_dir.iterdir())) == 5
|
||||
|
||||
|
||||
class TestGenerateHooksConfig:
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
"""Tests for unified-review MCP tool wiring (registration + docs sections)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent))
|
||||
|
||||
from code_review_graph.tools import ( # noqa: E402
|
||||
dedupe_findings_func,
|
||||
generate_report_func,
|
||||
score_review_func,
|
||||
)
|
||||
from code_review_graph.tools.docs import get_docs_section # noqa: E402
|
||||
|
||||
|
||||
class TestToolRegistration:
|
||||
def test_scoring_funcs_exported(self):
|
||||
assert callable(score_review_func)
|
||||
assert callable(dedupe_findings_func)
|
||||
assert callable(generate_report_func)
|
||||
|
||||
def test_mcp_tools_exposed(self):
|
||||
import code_review_graph.main as m
|
||||
|
||||
for name in (
|
||||
"score_review_tool",
|
||||
"dedupe_findings_tool",
|
||||
"generate_report_tool",
|
||||
):
|
||||
assert hasattr(m, name), f"{name} not exposed by main module"
|
||||
|
||||
|
||||
class TestDocsSections:
|
||||
def test_unified_review_section(self):
|
||||
result = get_docs_section("unified-review")
|
||||
assert result["status"] == "ok"
|
||||
assert "score_review_tool" in result["content"]
|
||||
|
||||
def test_score_review_section(self):
|
||||
result = get_docs_section("score-review")
|
||||
assert result["status"] == "ok"
|
||||
assert "dedupe_findings_tool" in result["content"]
|
||||
|
||||
def test_unknown_section(self):
|
||||
result = get_docs_section("does-not-exist")
|
||||
assert result["status"] == "not_found"
|
||||
assert "unified-review" in result["error"]
|
||||
|
||||
|
||||
class TestGenerateSkills:
|
||||
def test_unified_review_generated(self, tmp_path):
|
||||
from code_review_graph.skills import generate_skills
|
||||
|
||||
skills_dir = generate_skills(tmp_path)
|
||||
skill_file = skills_dir / "unified-review" / "SKILL.md"
|
||||
assert skill_file.is_file()
|
||||
content = skill_file.read_text(encoding="utf-8")
|
||||
assert "score_review_tool" in content
|
||||
assert "get_minimal_context" in content
|
||||
assert "detail_level" in content
|
||||
|
||||
def test_uninstall_knows_unified_review(self):
|
||||
from code_review_graph.uninstall import _generated_skill_slugs
|
||||
|
||||
assert "unified-review" in _generated_skill_slugs()
|
||||
Reference in New Issue
Block a user