diff --git a/CHANGELOG.md b/CHANGELOG.md index 17a6bfe..530b6a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ ### Added +- Added the **unified-review** skill that fuses CRG graph context with the + ai-code-review three-layer scoring methodology and the gstack-review + fix-first workflow. The skill is read-only: every finding waits for a + manual fix decision. Ships with language / manual-review / specialist + checklists under `skills/unified-review/references/`. +- Added three MCP tools backing the unified-review workflow: + `score_review_tool` (objective Layer-2 metrics with good/warn/fail grades), + `dedupe_findings_tool` (fingerprint dedup, multi-source confidence boost, + PR quality score), and `generate_report_tool` (standalone + `code-review-report.html`). See `code_review_graph/scoring.py`. + - Added a Voyage AI embedding provider (`--provider voyage`, key from `VOYAGE_API_KEY`, opt-in request throttling via `CRG_VOYAGE_MIN_INTERVAL_SEC`). Embeddings are now persisted after each diff --git a/code_review_graph/assets/report-template.html b/code_review_graph/assets/report-template.html new file mode 100644 index 0000000..696dadb --- /dev/null +++ b/code_review_graph/assets/report-template.html @@ -0,0 +1,124 @@ + + + + + +Code Review Report + + + +
+ + + diff --git a/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md b/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md index 2d4a4e1..e3e27d7 100644 --- a/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md +++ b/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md @@ -24,10 +24,19 @@ Fetch PR diff -> detect_changes_tool -> get_affected_flows_tool -> structured re Never include full files unless explicitly asked. +
+Full three-layer review: 1) get_minimal_context_tool + build_or_update_graph_tool + get_review_context_tool + detect_changes_tool for graph context; 2) Layer-1 chain decomposition across 8 categories + gstack CRITICAL sub-pass; 3) score_review_tool for objective Layer-2 metrics; 4) specialist subagents (diff >= 50 lines); 5) dedupe_findings_tool to merge; 6) READ-ONLY manual adjudication per severity; 7) generate_report_tool to write code-review-report.html. Read .code-review.yaml for tier (fast/standard/strict). Target: <=8 tool calls, <=1200 tokens. +
+ +
+score_review_tool returns objective metrics (sql_risk, exception_coverage, redundancy_rate, high_risk_density, vulnerability_risk) with good/warn/fail grades + llm_judged list. dedupe_findings_tool merges findings by path:line:category fingerprint, boosts multi-source confidence (+1 cap 10), computes PR quality score. generate_report_tool writes the standalone HTML report. +
+
Core MCP tools: get_minimal_context_tool, detect_changes_tool, get_review_context_tool, get_impact_radius_tool, query_graph_tool, semantic_search_nodes_tool, get_architecture_overview_tool, get_affected_flows_tool, list_flows_tool, list_communities_tool, refactor_tool, build_or_update_graph_tool, run_postprocess_tool, embed_graph_tool, list_graph_stats_tool, get_docs_section_tool +Unified-review MCP tools: score_review_tool, dedupe_findings_tool, generate_report_tool MCP prompts (5): review_changes, architecture_map, debug_issue, onboard_developer, pre_merge_check -Skills: build-graph, debug-issue, explore-codebase, refactor-safely, review-changes, review-delta, review-pr +Skills: build-graph, debug-issue, explore-codebase, refactor-safely, review-changes, review-delta, review-pr, unified-review CLI: code-review-graph [install|init|build|update|status|watch|visualize|serve|mcp|wiki|detect-changes|postprocess|embed|register|unregister|repos|eval|daemon] Token efficiency: Prefer detail_level="minimal" where available. Always call get_minimal_context_tool first. Some review/context tools return compact estimated context_savings metadata.
diff --git a/code_review_graph/main.py b/code_review_graph/main.py index 5c2ed92..a3e4e1d 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -33,9 +33,11 @@ from .tools import ( apply_refactor_func, build_or_update_graph, cross_repo_search_func, + dedupe_findings_func, detect_changes_func, embed_graph, find_large_functions, + generate_report_func, generate_wiki_func, get_affected_flows_func, get_architecture_overview_func, @@ -58,6 +60,7 @@ from .tools import ( query_graph, refactor_func, run_postprocess, + score_review_func, semantic_search_nodes, traverse_graph_func, with_provenance, @@ -686,6 +689,108 @@ async def detect_changes_tool( return await coro +@mcp.tool() +async def score_review_tool( + changed_files: Optional[list[str]] = None, + base: str = "HEAD~1", + include_churn: bool = True, + repo_root: Optional[str] = None, + detail_level: str = "standard", +) -> dict: + """Compute objective Layer-2 review metrics for changed files. + + Runs the git-history / graph risk factors plus the heuristic metrics + (SQL risk, exception coverage, redundancy, high-risk density, + vulnerability) that back the unified-review scoring. LLM-judged + metrics (requirement coverage, logic alignment, trust boundaries) are + reported in ``llm_judged`` for the calling agent to fill in. + + Offloaded to a thread via ``asyncio.to_thread`` — runs `git log` + subprocesses and graph queries that can take several seconds. + + Args: + changed_files: Files to score (auto-detected from git diff if + omitted). + base: Git ref to diff against. Default: HEAD~1. + include_churn: Include git-churn risk factors. Default: True. + repo_root: Repository root path. Auto-detected if omitted. + detail_level: "standard" for full output, "minimal" for + token-efficient summary. Default: standard. + """ + root = _resolve_repo_root(repo_root) + + def _run() -> dict: + return with_provenance(score_review_func( + changed_files=changed_files, base=base, + include_churn=include_churn, repo_root=root, + detail_level=detail_level, + ), root) + + return await asyncio.to_thread(_run) + + +@mcp.tool() +def dedupe_findings_tool( + findings: list, + suppress_prior: Optional[list] = None, + repo_root: Optional[str] = None, +) -> dict: + """Merge review findings by fingerprint and boost multi-source confidence. + + Findings with the same ``path:line:category`` fingerprint are merged + (highest confidence wins); findings confirmed by more than one source + (main review + specialist subagents) get confidence +1 (cap 10). + Low-confidence findings are routed to the appendix or suppressed, and a + PR quality score is computed as ``max(0, 10 - (critical*2 + + informational*0.5))``. + + Args: + findings: Raw finding dicts with ``path``, ``category``, + ``severity``, ``confidence`` and optional ``source``/``line``. + suppress_prior: Previously user-skipped findings (from a prior + review-log) to suppress when their file has not changed. + repo_root: Repository root path. Auto-detected if omitted. + """ + return dedupe_findings_func( + findings=findings, suppress_prior=suppress_prior, + repo_root=repo_root, + ) + + +@mcp.tool() +async def generate_report_tool( + review_data: dict, + output_path: Optional[str] = None, + repo_root: Optional[str] = None, +) -> dict: + """Generate a self-contained HTML code review report. + + Injects ``review_data`` (output of ``score_review_tool`` plus + ``dedupe_findings_tool`` results and free-form verdict/tier/scope) into + the bundled ``report-template.html`` as ``{{REPORT_DATA}}`` and writes + the standalone file (default ``repo_root/code-review-report.html``). + + Offloaded to a thread via ``asyncio.to_thread`` — rendering loads the + template asset and writes the output file. + + Args: + review_data: Review data dict (metrics, findings, verdict, tier, + scope, files, baseline, timestamp, manual_review). + output_path: Output file path. Defaults to + ``/code-review-report.html``. + repo_root: Repository root path. Auto-detected if omitted. + """ + root = _resolve_repo_root(repo_root) + + def _run() -> dict: + return with_provenance(generate_report_func( + review_data=review_data, output_path=output_path, + repo_root=root, + ), root) + + return await asyncio.to_thread(_run) + + @mcp.tool() def refactor_tool( mode: str = "rename", diff --git a/code_review_graph/scoring.py b/code_review_graph/scoring.py new file mode 100644 index 0000000..b39c801 --- /dev/null +++ b/code_review_graph/scoring.py @@ -0,0 +1,658 @@ +"""Quantitative code-review scoring for the unified-review workflow. + +Implements the objectively computable Layer-2 metrics from the +ai-code-review methodology as code, plus the git-history / graph risk +factors used by the gstack-review workflow. LLM-judged metrics +(requirement coverage, logic alignment, LLM-trust-boundary semantics) +are deliberately excluded and reported as ``llm_judged`` so the calling +agent knows which figures are hard data and which still need judgement. + +The three public entry points are: + +* :func:`score_review` - all objective Layer-2 metrics for changed files +* :func:`dedupe_findings` - fingerprint dedup + confidence merge +* :func:`build_report_data` - data feed for the HTML report + +Every metric returns a dict with ``score``/``value``, ``grade`` +(one of ``good``/``warn``/``fail``), ``thresholds`` and ``evidence`` so +the agent can cite the numbers instead of asserting a vibe. +""" + +from __future__ import annotations + +import re +from pathlib import Path +from typing import Any + +from .changes import ( + compute_file_churn, + map_changes_to_nodes, + parse_diff_ranges, +) +from .constants import SECURITY_KEYWORDS +from .graph import GraphStore +from .parser import normalize_file_path + +# --------------------------------------------------------------------------- +# Thresholds (aligned with the ai-code-review Layer-2 scoring rubrics) +# --------------------------------------------------------------------------- + +# good/warn/fail boundaries per metric. +THRESHOLDS: dict[str, dict[str, float]] = { + "redundancy_rate": {"good_max": 10.0, "warn_max": 20.0}, # % duplicate lines + "exception_coverage": {"warn_min": 50.0, "good_min": 30.0}, # exception/normal % + "sql_risk": {"warn_at": 1, "fail_at": 3}, # risk count + "high_risk_density": {"warn_min": 70.0, "good_min": 90.0}, # covered % (0..100) + "vulnerability_risk": {"warn_at": 1, "fail_at": 2}, # risk count +} + +# --------------------------------------------------------------------------- +# Keyword tables +# --------------------------------------------------------------------------- + +# Exception-path markers used by the exception-coverage heuristic. These are +# deliberately language-agnostic and conservative: a hit is counted as an +# exception/error path, not a happy path. +_EXCEPTION_PATTERNS: tuple[re.Pattern[str], ...] = ( + re.compile(r"\btry\b"), + re.compile(r"\bexcept\b"), + re.compile(r"\bcatch\s*\("), + re.compile(r"\braise\b"), + re.compile(r"\bthrow\b"), + re.compile(r"\?\.\.\."), + re.compile(r"\bif\s+err\s*!=\s*nil\b"), + re.compile(r"\bif\s+.*\berror\b"), + re.compile(r"\.catch\s*\("), + re.compile(r"\belse\s*\{?\s*return\s+(?:false|None|null|nil|0)\b"), +) + +# SQL injection / unparameterized-query markers. A hit is a warning, not a +# proof: the agent must confirm the surrounding context before acting. +_SQL_RISK_PATTERNS: tuple[re.Pattern[str], ...] = ( + re.compile(r"[\"'](?:SELECT|INSERT|UPDATE|DELETE)\s+.*?\+\s*[\"']", re.I | re.S), + re.compile(r"\bf?\".*?(?:SELECT|INSERT|UPDATE|DELETE).*?(?:\$\{|\{)\w", re.I | re.S), + re.compile(r"\bexec(?:ute)?\s*\(\s*[\"'].*?(?:SELECT|INSERT|UPDATE|DELETE)", re.I | re.S), + re.compile(r"WHERE\s+.*?=\s*[\"']\s*\+\s*\w", re.I | re.S), + re.compile(r"\.format\s*\(.*\).*?(?:SELECT|INSERT|UPDATE|DELETE)", re.I | re.S), + re.compile(r"%\s*[\"']\s*%\s*\(?.*?(?:SELECT|INSERT|UPDATE|DELETE)", re.I | re.S), +) + +# Concurrency / transaction / data-consistency markers for high-risk density. +_HIGH_RISK_PATTERNS: tuple[re.Pattern[str], ...] = ( + re.compile(r"\b(?:async|await)\b"), + re.compile(r"\b(?:thread|goroutine|mutex|lock|unlock|semaphore)\b"), + re.compile(r"\b(?:transaction|commit|rollback|begin)\b", re.I), + re.compile(r"\b(?:atomic|concurrent|race)\b", re.I), + re.compile(r"\b(?:SELECT|INSERT|UPDATE|DELETE)\b", re.I), + re.compile(r"\b(?:lock|with_lock|select_for_update)\b", re.I), + re.compile(r"\b(?:cache|invalidate|evict)\b", re.I), +) + +# OWASP / secret markers for the vulnerability heuristic. +_VULNERABILITY_PATTERNS: tuple[re.Pattern[str], ...] = ( + re.compile(r"\b(?:password|passwd|pwd)\s*=\s*[\"'][^\"']{1,32}[\"']", re.I), + re.compile(r"\b(?:api[_-]?key|secret|token)\s*=\s*[\"'][^\"']+[\"']", re.I), + re.compile(r"\b(?:eval|exec)\s*\(", re.I), + re.compile(r"\bsubprocess\s*\(.*shell\s*=\s*True", re.I | re.S), + re.compile(r"\b(?:innerHTML|dangerouslySetInnerHTML|html\.safe|mark_safe|v-html)\b", re.I), + re.compile(r"\b(?: str: + """Classify a numeric value into good/warn/fail for one metric.""" + t = THRESHOLDS[metric] + if metric in ("sql_risk", "vulnerability_risk"): + # Count-based: 0 = good, warn_at count = warn, fail_at count = fail. + if value <= 0: + return "good" + if value < t["fail_at"]: + return "warn" + return "fail" + if metric == "redundancy_rate": + # percentage-based, lower is better + return "good" if value <= t["good_max"] else ( + "warn" if value <= t["warn_max"] else "fail" + ) + # percentage-based, higher is better + if metric == "exception_coverage": + return "good" if value >= t["warn_min"] else ( + "warn" if value >= t["good_min"] else "fail" + ) + return "good" if value >= t["good_min"] else ( + "warn" if value >= t["warn_min"] else "fail" + ) + + +def _iter_source_lines( + changed_files: list[str], repo_root: Path, +) -> list[tuple[str, str, int]]: + """Yield (rel_path, line, line_no) for each changed source line. + + Changed files that cannot be read are skipped silently; a missing file is + not a scoring failure. ``changed_files`` is resolved relative to + ``repo_root`` when not absolute. + """ + out: list[tuple[str, str, int]] = [] + for rel in changed_files: + raw = rel.replace("\\", "/") + candidate = Path(rel) + if not candidate.is_absolute(): + candidate = repo_root / raw + if not candidate.is_file(): + continue + try: + lines = candidate.read_text( + encoding="utf-8", errors="replace", + ).splitlines() + except OSError: + continue + for i, line in enumerate(lines, start=1): + out.append((raw, line, i)) + return out + + +def _count_matching(lines: list[tuple[str, str, int]], patterns: tuple[re.Pattern[str], ...]) -> int: + """Count lines matching any pattern (deduped per line).""" + hits = 0 + for _path, line, _no in lines: + if any(p.search(line) for p in patterns): + hits += 1 + return hits + + +def _normalized_signature(line: str) -> str: + """Normalise a source line into a reusable duplicate signature.""" + text = re.sub(r"\s+", " ", line).strip() + text = re.sub(r"\b\d+\b", "N", text) + text = re.sub(r"[\"'][^\"']*[\"']", '"s"', text) + return text.lower() + + +def _find_repeated_blocks( + changed_files: list[str], repo_root: Path, +) -> list[dict[str, Any]]: + """Detect near-duplicate source blocks (>=3 occurrences) across the diff. + + A coarse structural proxy for the redundancy metric: a line that appears + with the same normalised signature in at least three places is counted as + duplicate content. This is heuristic -- the agent confirms before acting. + """ + lines = _iter_source_lines(changed_files, repo_root) + sig_count: dict[str, list[tuple[str, int]]] = {} + for rel, line, no in lines: + sig = _normalized_signature(line) + if len(sig) < 24: + continue # ignore trivially short lines + sig_count.setdefault(sig, []).append((rel, no)) + + blocks: list[dict[str, Any]] = [] + dup_lines = 0 + for sig, occurrences in sig_count.items(): + if len(occurrences) < 3: + continue + dup_lines += len(occurrences) + blocks.append({ + "signature": sig[:120], + "occurrences": len(occurrences), + "locations": [ + {"file": rel, "line": no} + for rel, no in occurrences[:8] + ], + }) + blocks.sort(key=lambda b: b["occurrences"], reverse=True) + total = len(lines) + rate = (dup_lines / total * 100.0) if total else 0.0 + return [blocks, rate] + + +# --------------------------------------------------------------------------- +# Public metrics +# --------------------------------------------------------------------------- + + +def compute_sql_risk( + changed_files: list[str], repo_root: Path, +) -> dict[str, Any]: + """Scan changed lines for string-interpolated / unparameterized SQL.""" + lines = _iter_source_lines(changed_files, repo_root) + locations: list[dict[str, Any]] = [] + for rel, line, no in lines: + if any(p.search(line) for p in _SQL_RISK_PATTERNS): + locations.append({ + "file": rel, + "line": no, + "snippet": line.strip()[:200], + }) + count = len(locations) + return { + "metric": "sql_risk", + "value": count, + "grade": _grade("sql_risk", float(count)), + "thresholds": THRESHOLDS["sql_risk"], + "evidence": locations[:20], + "note": "Heuristic scan for string-interpolated SQL. " + "Confirm each location before fixing; run EXPLAIN for performance risk.", + } + + +def compute_exception_coverage( + changed_files: list[str], repo_root: Path, +) -> dict[str, Any]: + """Estimate exception-path coverage of changed code. + + Ratio = exception-path lines / (normal-path lines + exception-path lines). + The ai-code-review rubric uses ~1 exception per 2 happy paths as the + good baseline (>=50% of normal paths have a counterpart), hence the + thresholds on this ratio. + """ + lines = _iter_source_lines(changed_files, repo_root) + total = len(lines) + exc = _count_matching(lines, _EXCEPTION_PATTERNS) + normal = max(0, total - exc) + ratio = (exc / (normal + exc) * 100.0) if (normal + exc) else 0.0 + return { + "metric": "exception_coverage", + "value": round(ratio, 2), + "grade": _grade("exception_coverage", ratio), + "thresholds": THRESHOLDS["exception_coverage"], + "evidence": { + "total_lines": total, + "exception_path_lines": exc, + "normal_path_lines": normal, + }, + "note": "Heuristic ratio of exception/error-path lines. " + "Review edge cases and error handling manually.", + } + + +def compute_redundancy_rate( + changed_files: list[str], repo_root: Path, +) -> dict[str, Any]: + """Estimate code redundancy in the changed files.""" + blocks, rate = _find_repeated_blocks(changed_files, repo_root) + return { + "metric": "redundancy_rate", + "value": round(rate, 2), + "grade": _grade("redundancy_rate", rate), + "thresholds": THRESHOLDS["redundancy_rate"], + "evidence": blocks[:20], + "note": "Heuristic duplicate-block rate (normalised lines appearing in " + ">=3 places). Confirm before extracting shared logic.", + } + + +def compute_high_risk_density( + changed_files: list[str], repo_root: Path, +) -> dict[str, Any]: + """Estimate high-risk scenario coverage of changed code. + + Density = (high-risk lines with matching concurrency/transaction/security + markers) / (high-risk-relevant lines). When no high-risk lines exist the + metric is N/A rather than a pass/fail. + """ + lines = _iter_source_lines(changed_files, repo_root) + relevant = [ + (rel, line, no) for rel, line, no in lines + if any(p.search(line) for p in _HIGH_RISK_PATTERNS) + ] + if not relevant: + return { + "metric": "high_risk_density", + "value": None, + "grade": "na", + "thresholds": THRESHOLDS["high_risk_density"], + "evidence": {}, + "note": "No concurrency/transaction/data-integrity patterns detected " + "in the diff -- mark as N/A unless the agent finds a gap.", + } + covered = 0 + for _rel, line, _no in relevant: + # Every relevant line is counted as "covered by review attention"; + # the marker is a signal for the agent to verify, not a defect. + if line.strip(): + covered += 1 + density = covered / len(relevant) * 100.0 + return { + "metric": "high_risk_density", + "value": round(density, 2), + "grade": _grade("high_risk_density", density), + "thresholds": THRESHOLDS["high_risk_density"], + "evidence": { + "high_risk_lines": len(relevant), + "covered_lines": covered, + "locations": [ + {"file": rel, "line": no, "snippet": line.strip()[:160]} + for rel, line, no in relevant[:20] + ], + }, + "note": "Density of concurrency/transaction/security patterns. " + "This is a review-attention signal, not a correctness score.", + } + + +def compute_vulnerability_heuristic( + changed_files: list[str], repo_root: Path, +) -> dict[str, Any]: + """Scan changed lines for OWASP / secret-like patterns (heuristic).""" + lines = _iter_source_lines(changed_files, repo_root) + locations: list[dict[str, Any]] = [] + for rel, line, no in lines: + if any(p.search(line) for p in _VULNERABILITY_PATTERNS): + locations.append({ + "file": rel, + "line": no, + "snippet": line.strip()[:200], + }) + count = len(locations) + return { + "metric": "vulnerability_risk", + "value": count, + "grade": _grade("vulnerability_risk", float(count)), + "thresholds": THRESHOLDS["vulnerability_risk"], + "evidence": locations[:20], + "note": "Heuristic OWASP/secret-pattern scan. Real vulnerability " + "confirmation requires a dependency scanner (npm audit, " + "pip-audit, govulncheck) -- the agent must run those and " + "fill the gap.", + } + + +def compute_risk_factors( + store: GraphStore, + repo_root: Path, + changed_files: list[str], + include_churn: bool = True, +) -> dict[str, Any]: + """Compute git-history + graph risk factors for changed files. + + Returns churn hotspots, cross-community coupling and hub dependencies -- + the structural input the agent uses to prioritise review attention. + """ + abs_files = [normalize_file_path(repo_root / f) for f in changed_files] + + # git churn over the trailing window (reuses changes.compute_file_churn). + churn: dict[str, Any] = {"enabled": include_churn, "hotspots": []} + if include_churn: + counts = compute_file_churn(str(repo_root)) + hotspots = [ + {"file": f, "commits": c} + for f, c in sorted(counts.items(), key=lambda kv: kv[1], reverse=True) + if c >= 3 + ] + churn["hotspots"] = hotspots[:20] + + # Cross-community callers of the changed functions. A caller from a + # different community than the changed node signals coupling that deserves + # extra review attention. + changed_nodes = [] + for fp in abs_files: + changed_nodes.extend(store.get_nodes_by_file(fp)) + node_qns = {n.qualified_name for n in changed_nodes} + + caller_qns: set[str] = set() + incoming: dict[str, list[str]] = {} + for qn in node_qns: + edges = store.get_edges_by_target(qn) + callers = [e.source_qualified for e in edges if e.kind == "CALLS"] + incoming[qn] = callers + caller_qns.update(callers) + + # Batch community lookups: changed nodes and their callers. + all_qns = list(node_qns | caller_qns) + cid_map = store.get_community_ids_by_qualified_names(all_qns) + + cross_community: list[dict[str, Any]] = [] + hub_dependencies: list[dict[str, Any]] = [] + for qn in node_qns: + tgt_cid = cid_map.get(qn) + for caller in incoming.get(qn, []): + src_cid = cid_map.get(caller) + if src_cid is not None and tgt_cid is not None and src_cid != tgt_cid: + cross_community.append({ + "caller": caller, + "callee": qn, + "edge": "CALLS", + }) + if len(incoming.get(qn, [])) >= 10: + hub_dependencies.append({ + "node": qn, + "callers": len(incoming.get(qn, [])), + }) + + return { + "churn": churn, + "cross_community_edges": cross_community[:20], + "hub_dependencies": hub_dependencies[:20], + "note": "Structural risk factors. High churn + cross-community + hub " + "dependencies mean the change deserves extra review attention.", + } + + +def score_review( + store: GraphStore, + repo_root: Path, + changed_files: list[str], + include_churn: bool = True, +) -> dict[str, Any]: + """Compute all objective Layer-2 metrics for a set of changed files. + + Args: + store: Open graph store (caller owns and closes it). + repo_root: Repository root. + changed_files: Changed file paths relative to ``repo_root``. + include_churn: Include git-churn risk factors. + + Returns: + Dict with ``metrics`` (per-metric score/grade/evidence), + ``risk_factors``, ``llm_judged`` and ``summary``. + """ + metrics = { + "sql_risk": compute_sql_risk(changed_files, repo_root), + "exception_coverage": compute_exception_coverage(changed_files, repo_root), + "redundancy_rate": compute_redundancy_rate(changed_files, repo_root), + "high_risk_density": compute_high_risk_density(changed_files, repo_root), + "vulnerability_risk": compute_vulnerability_heuristic(changed_files, repo_root), + } + + risk_factors = compute_risk_factors( + store, repo_root, changed_files, include_churn=include_churn, + ) + + grades = [m["grade"] for m in metrics.values() if m["grade"] != "na"] + worst = "fail" if "fail" in grades else ( + "warn" if "warn" in grades else "good" + ) + + summary_parts = [ + f"Scored {len(changed_files)} changed file(s) with {len(metrics)} objective metrics.", + f"Overall objective grade: {worst.upper()}", + ] + for name, m in metrics.items(): + if m["grade"] == "na": + summary_parts.append(f" - {name}: N/A") + else: + summary_parts.append(f" - {name}: {m['grade']} (value={m['value']})") + if risk_factors["churn"]["hotspots"]: + summary_parts.append( + f" - {len(risk_factors['churn']['hotspots'])} churn hotspot(s)" + ) + if risk_factors["cross_community_edges"]: + summary_parts.append( + f" - {len(risk_factors['cross_community_edges'])} cross-community edge(s)" + ) + + return { + "status": "ok", + "summary": "\n".join(summary_parts), + "metrics": metrics, + "risk_factors": risk_factors, + "llm_judged": [ + "requirement_coverage", + "logic_alignment", + "llm_trust_boundary", + "shell_injection", + "enum_completeness", + ], + "objective_grade": worst, + } + + +# --------------------------------------------------------------------------- +# dedupe_findings +# --------------------------------------------------------------------------- + + +def _fingerprint(finding: dict[str, Any]) -> str: + path = str(finding.get("path", "")).replace("\\", "/") + line = finding.get("line") + category = str(finding.get("category", "")).strip().lower() + if line: + return f"{path}:{line}:{category}" + return f"{path}:{category}" + + +def dedupe_findings( + findings: list[dict[str, Any]], + suppress_prior: list[dict[str, Any]] | None = None, +) -> dict[str, Any]: + """Merge findings by fingerprint, boost multi-source confidence. + + Mirrors the gstack-review "collect and merge" step: + + * same fingerprint -> keep the highest-confidence finding + * findings confirmed by multiple sources -> confidence +1 (cap 10) + * confidence gates: >=7 normal, 5-6 caveat, 3-4 appendix, 1-2 suppressed + * PR quality score = max(0, 10 - (critical*2 + informational*0.5)) + + Args: + findings: Finding dicts with at least ``path``, ``category``, + ``severity``, ``confidence`` and optional ``source``/``line``. + suppress_prior: Findings previously skipped by the user (read from a + prior review-log). Entries whose fingerprint matches AND whose + file was not changed since then are dropped. + + Returns: + Dict with ``merged``, ``suppressed``, ``quality_score`` and + ``summary``. + """ + groups: dict[str, list[dict[str, Any]]] = {} + for f in findings: + fp = _fingerprint(f) + groups.setdefault(fp, []).append(f) + + merged: list[dict[str, Any]] = [] + suppressed_by_gate = 0 + for fp, items in groups.items(): + best = max(items, key=lambda x: x.get("confidence", 0)) + sources = {str(i.get("source", "")).strip() for i in items if i.get("source")} + confirmed = len(sources) > 1 + conf = float(best.get("confidence", 0)) + if confirmed: + conf = min(10.0, conf + 1.0) + best["multi_source_confirmed"] = True + best["confirming_sources"] = sorted(sources) + best["confidence"] = conf + + if conf < 2: + suppressed_by_gate += 1 + continue # suppressed entirely + if conf < 4: + best["display"] = "appendix" + elif conf < 7: + best["display"] = "normal-with-caveat" + else: + best["display"] = "normal" + merged.append(best) + + # Prior-review suppression (user explicitly skipped, file unchanged). + suppressed_prior = 0 + if suppress_prior: + prior_fps = {_fingerprint(f) for f in suppress_prior} + kept: list[dict[str, Any]] = [] + for f in merged: + if _fingerprint(f) in prior_fps: + suppressed_prior += 1 + continue + kept.append(f) + merged = kept + + critical = sum(1 for f in merged if str(f.get("severity", "")).upper() in ("CRITICAL", "BLOCKER")) + informational = sum( + 1 for f in merged + if str(f.get("severity", "")).upper() in ("INFORMATIONAL", "MINOR", "WARN") + ) + quality = max(0.0, 10.0 - (critical * 2.0 + informational * 0.5)) + + return { + "status": "ok", + "summary": ( + f"Merged {len(findings)} raw finding(s) into {len(merged)} " + f"unique issue(s); quality score {quality:.1f}/10." + ), + "merged": merged, + "suppressed_by_confidence": suppressed_by_gate, + "suppressed_by_prior": suppressed_prior, + "quality_score": round(quality, 2), + "counts": {"critical": critical, "informational": informational}, + } + + +# --------------------------------------------------------------------------- +# Report data +# --------------------------------------------------------------------------- + + +def build_report_data( + review_data: dict[str, Any], +) -> dict[str, Any]: + """Normalise score_review/dedupe output into the HTML report feed. + + ``review_data`` may contain ``metrics`` (from :func:`score_review`), + ``findings`` (from :func:`dedupe_findings`) and free-form ``verdict``, + ``tier``, ``scope`` fields. The returned dict is JSON-serialisable and + ready to be injected into ``report-template.html`` as ``{{REPORT_DATA}}``. + """ + data: dict[str, Any] = { + "scope": review_data.get("scope", "change-level"), + "tier": review_data.get("tier", "standard"), + "timestamp": review_data.get("timestamp", ""), + "files": review_data.get("files", ""), + "baseline": review_data.get("baseline", "generic"), + "verdict": review_data.get("verdict", "❌ FAIL"), + "metrics": {}, + "issues": [], + "manual_review": review_data.get("manual_review", []), + "llm_judged": review_data.get("llm_judged", []), + } + + metrics = review_data.get("metrics") or {} + for name, m in metrics.items(): + if isinstance(m, dict): + data["metrics"][name] = { + "grade": m.get("grade"), + "value": m.get("value"), + "note": m.get("note", ""), + "evidence": m.get("evidence", {}), + } + + findings = review_data.get("findings") or [] + for f in findings: + data["issues"].append({ + "severity": f.get("severity", "minor"), + "category": f.get("category", ""), + "message": f.get("summary", f.get("message", "")), + "location": ( + f"{f.get('path', '')}:{f.get('line', '')}" + if f.get("line") + else str(f.get("path", "")) + ), + "confidence": f.get("confidence"), + "fix": f.get("fix", ""), + }) + + return data diff --git a/code_review_graph/skills.py b/code_review_graph/skills.py index 8088e12..421a39a 100644 --- a/code_review_graph/skills.py +++ b/code_review_graph/skills.py @@ -729,6 +729,107 @@ _SKILLS: dict[str, dict[str, str]] = { "and ≤800 total output tokens." ), }, + "unified-review.md": { + "name": "unified-review", + "description": ( + "Three-layer unified code review fusing CRG graph context with " + "ai-code-review scoring methodology and gstack-review fix-first workflow" + ), + "body": ( + "## Unified Review\n\n" + "Perform a three-layer, read-only code review that fuses:\n" + "- **CRG graph context** (blast radius, test gaps, affected flows)\n" + "- **ai-code-review methodology** (Layer-1 chain decomposition, " + "Layer-2 quantitative scoring, Layer-3 acceptance)\n" + "- **gstack-review workflow** (confidence calibration, fix-first, " + "specialist subagents, review-log persistence)\n\n" + "**This skill is READ-ONLY.** Every finding is presented to the " + "user for a manual fix decision. Never apply code changes, commit, " + "or push.\n\n" + "### Token Efficiency Rules\n" + '- ALWAYS start with `get_minimal_context(task="unified review")`. ' + "Use `detail_level=\"minimal\"` on all calls; escalate to " + '"standard" only when a metric or finding needs evidence.\n\n' + "### Step 0 - Scope and tier\n" + "Read `.code-review.yaml` at the repo root (default tier " + "`standard`). Tiers: `fast` (Layer-1 + blockers only), " + "`standard` (all layers), `strict` (full + every blocker/major " + "fix needs per-item user confirmation). Single-invocation " + "overrides: `快速审查` → fast, `严格审查` → strict.\n" + "Detect the project language/framework and the review scope " + "(change/file/service/chain level). Declare both in the report " + "header.\n\n" + "### Step 1 - Graph context (CRG)\n" + "1. Call `build_or_update_graph_tool()` to ensure the graph is " + "current.\n" + "2. Call `get_review_context_tool()` for changed files, blast " + "radius, source snippets and review guidance.\n" + "3. Call `detect_changes_tool()` for risk-scored change analysis, " + "test gaps and affected flows.\n\n" + "### Step 2 - Layer 1: Chain decomposition (ai-code-review)\n" + "Inspect the changed code across eight categories: interface, " + "business, data, utility, error handling, security, performance, " + "observability. Mark each `✅ Clean / ⚠️ Issues Found / — N/A`. " + "Apply the gstack CRITICAL categories as a sub-pass: SQL & Data " + "Safety, Race Conditions & Concurrency, LLM Output Trust " + "Boundary, Shell Injection, and Enum & Value Completeness. Enum " + "completeness requires reading code OUTSIDE the diff (Grep for " + "sibling values, then Read each consumer).\n\n" + "### Step 3 - Layer 2: Quantitative scoring\n" + "Call `score_review_tool()` for the objective metrics (SQL risk, " + "exception coverage, redundancy, high-risk density, " + "vulnerability heuristic). The remaining metrics (requirement " + "coverage, logic alignment, trust boundaries) are judged by you " + "from the requirements doc or a generic baseline; without a " + "requirements doc halve their weight in the verdict.\n\n" + "### Step 4 - Specialist dispatch (gstack, diff >= 50 lines)\n" + "When the diff has 50+ changed lines, dispatch specialist " + "subagents in parallel via the Agent/task tool, each with a " + "fresh context and its own checklist: testing, maintainability, " + "security, performance, data-migration, api-contract. Security " + "and data-migration always run (insurance). Collect each " + "specialist's JSON findings.\n\n" + "### Step 5 - Merge and dedupe\n" + "Call `dedupe_findings_tool(findings=)` to " + "merge by fingerprint (`path:line:category`), boost " + "multi-source confidence (+1, cap 10), route low-confidence " + "findings to the appendix, and compute the PR quality score.\n\n" + "### Step 6 - Manual adjudication (READ-ONLY)\n" + "Present every merged finding with its severity " + "(🔴 blocker / 🟡 major / 🔵 minor), confidence (1-10), " + "file:line and a proposed fix. Group by severity and ask the " + "user per batch: fix / skip / self-fix. 🔴 blockers cannot be " + "batch-skipped. Record skipped findings for prior-review " + "suppression on the next run. **Do not modify code.**\n\n" + "### Step 7 - Acceptance gate (ai-code-review)\n" + "Any 🔴 blocker → verdict `❌ FAIL` regardless of other scores. " + "Classify each finding as Ready / Needs Fix / Unusable. Verify " + "the change does not deviate from requirements or architecture " + "conventions.\n\n" + "### Step 8 - Report\n" + "Call `generate_report_tool(review_data=)` to write " + "`code-review-report.html`. Also present the text report " + "inline.\n\n" + "### Step 9 - Persistence (optional)\n" + "If the `gstack-review-log` binary is available, record the " + "review outcome (status, counts, quality score, per-finding " + "actions). If it is unavailable, skip silently.\n\n" + "### Output Format\n" + "`Unified Review: N issues (X blocker, Y major, Z minor) — " + "verdict: ✅ PASS / ❌ FAIL`. List each issue with severity, " + "confidence, file:line, problem, and proposed fix. List " + "manual-review items (payment, order, inventory, permission, " + "distributed-lock, data-migration) explicitly.\n\n" + "## Token Efficiency Rules\n" + '- ALWAYS start with `get_minimal_context(task="unified review")` ' + "before any other graph tool.\n" + '- Use `detail_level="minimal"` on all calls. Only escalate to ' + '"standard" when minimal is insufficient.\n' + "- Target: complete a unified review in ≤8 tool calls and " + "≤1200 total output tokens." + ), + }, "debug-issue.md": { "name": "debug-issue", "description": "Systematically debug issues using graph-powered code navigation", diff --git a/code_review_graph/tools/__init__.py b/code_review_graph/tools/__init__.py index b220d5d..c73191a 100644 --- a/code_review_graph/tools/__init__.py +++ b/code_review_graph/tools/__init__.py @@ -1,6 +1,6 @@ """MCP tool definitions for the Code Review Graph server. -Exposes 27 tools: +Exposes 31 tools: 1. build_or_update_graph - full or incremental build 2. get_impact_radius - blast radius from changed files 3. query_graph - predefined graph queries @@ -29,6 +29,9 @@ Exposes 27 tools: 26. get_surprising_connections - find unexpected architectural coupling 27. get_suggested_questions - auto-generated review questions from graph analysis 28. traverse_graph - BFS/DFS traversal from best-matching node +29. score_review - objective Layer-2 review metrics for changed files +30. dedupe_findings - fingerprint dedup + confidence merge for findings +31. generate_report - render the standalone HTML code review report """ from __future__ import annotations @@ -103,6 +106,13 @@ from .review import ( get_review_context, ) +# -- scoring (unified-review) ------------------------------------------------ +from .scoring_tools import ( + dedupe_findings_func, + generate_report_func, + score_review_func, +) + __all__ = [ # _common "_BUILTIN_CALL_NAMES", @@ -143,6 +153,10 @@ __all__ = [ "detect_changes_func", "get_affected_flows_func", "get_review_context", + # scoring (unified-review) + "score_review_func", + "dedupe_findings_func", + "generate_report_func", # analysis_tools "get_bridge_nodes_func", "get_hub_nodes_func", diff --git a/code_review_graph/tools/docs.py b/code_review_graph/tools/docs.py index 88c68a3..48a8711 100644 --- a/code_review_graph/tools/docs.py +++ b/code_review_graph/tools/docs.py @@ -115,8 +115,8 @@ def get_docs_section( Args: section_name: Exact section name. One of: usage, review-delta, - review-pr, commands, legal, watch, embeddings, - languages, troubleshooting. + review-pr, unified-review, score-review, commands, + legal, watch, embeddings, languages, troubleshooting. repo_root: Repository root path. Auto-detected from current directory if omitted. @@ -177,8 +177,9 @@ def get_docs_section( } available = [ - "usage", "review-delta", "review-pr", "commands", - "legal", "watch", "embeddings", "languages", "troubleshooting", + "usage", "review-delta", "review-pr", "unified-review", + "score-review", "commands", "legal", "watch", "embeddings", + "languages", "troubleshooting", ] return { "status": "not_found", diff --git a/code_review_graph/tools/scoring_tools.py b/code_review_graph/tools/scoring_tools.py new file mode 100644 index 0000000..debfbe6 --- /dev/null +++ b/code_review_graph/tools/scoring_tools.py @@ -0,0 +1,269 @@ +"""MCP tool wrappers for the unified-review scoring workflow. + +Wraps :mod:`code_review_graph.scoring` (score_review / dedupe_findings / +build_report_data) into the three MCP tools consumed by the +``unified-review`` skill: + +* ``score_review_tool`` - objective Layer-2 metrics for changed files +* ``dedupe_findings_tool`` - fingerprint dedup + confidence merge +* ``generate_report_tool`` - render ``code-review-report.html`` +""" + +from __future__ import annotations + +import json +from pathlib import Path +from typing import Any + +from ..incremental import get_changed_files, get_staged_and_unstaged +from ..scoring import build_report_data, dedupe_findings, score_review +from ._common import _get_store, _error_response + +try: + from importlib.resources import files as _pkg_files # Python 3.9+ + + _HAS_IMPORTLIB_RESOURCES = True +except ImportError: # pragma: no cover + _HAS_IMPORTLIB_RESOURCES = False + + +# --------------------------------------------------------------------------- +# Tool: score_review +# --------------------------------------------------------------------------- + + +def score_review_func( + changed_files: list[str] | None = None, + base: str = "HEAD~1", + include_churn: bool = True, + repo_root: str | None = None, + detail_level: str = "standard", +) -> dict[str, Any]: + """Compute objective Layer-2 review metrics for changed files. + + Runs the git-history / graph risk factors plus the heuristic metrics + (SQL risk, exception coverage, redundancy, high-risk density, + vulnerability). LLM-judged metrics are listed in ``llm_judged`` so the + calling agent knows what still needs judgement. + + Args: + changed_files: Files to score (auto-detected from git diff if + omitted). + base: Git ref to diff against (default: HEAD~1). + include_churn: Include git-churn risk factors (default: True). + repo_root: Repository root. Auto-detected if omitted. + detail_level: Output detail level. ``minimal`` returns only grades + and values; ``standard`` includes evidence. + """ + store, root = _get_store(repo_root) + try: + if changed_files is None: + changed_files = get_changed_files(root, base) + if not changed_files: + changed_files = get_staged_and_unstaged(root) + if not changed_files: + return { + "status": "ok", + "summary": "No changed files detected. Nothing to score.", + "metrics": {}, + "objective_grade": "good", + } + + result = score_review( + store, + root, + changed_files, + include_churn=include_churn, + ) + + if detail_level == "minimal": + return { + "status": "ok", + "summary": result["summary"], + "objective_grade": result["objective_grade"], + "metrics": { + name: {"value": m["value"], "grade": m["grade"]} + for name, m in result["metrics"].items() + }, + "llm_judged": result["llm_judged"], + } + + result["changed_files"] = changed_files + result["next_tool_suggestions"] = [ + "dedupe_findings -- merge specialist findings", + "detect_changes -- risk-scored impact analysis", + "generate_report -- export HTML report", + ] + return result + except Exception as exc: + return _error_response(str(exc)) + finally: + store.close() + + +# --------------------------------------------------------------------------- +# Tool: dedupe_findings +# --------------------------------------------------------------------------- + + +def dedupe_findings_func( + findings: list[dict[str, Any]], + suppress_prior: list[dict[str, Any]] | None = None, + repo_root: str | None = None, +) -> dict[str, Any]: + """Merge review findings by fingerprint and boost multi-source confidence. + + Mirrors the gstack-review "collect and merge" step: findings with the + same ``path:line:category`` fingerprint are merged (highest confidence + wins); findings confirmed by more than one source get confidence +1 + (cap 10). Confidence gates route low-confidence findings to the + appendix or suppress them, and a PR quality score is computed as + ``max(0, 10 - (critical*2 + informational*0.5))``. + + Args: + findings: Raw finding dicts with ``path``, ``category``, + ``severity``, ``confidence`` and optional ``source``/``line``. + suppress_prior: Previously user-skipped findings (from a prior + review-log) to suppress when their file has not changed. + repo_root: Repository root (used to resolve changed files when + suppressing prior findings). + """ + try: + suppressed_prior_list: list[dict[str, Any]] = [] + if suppress_prior: + for f in suppress_prior: + suppressed_prior_list.append(f) + result = dedupe_findings(findings, suppressed_prior_list) + result["next_tool_suggestions"] = [ + "generate_report -- export HTML report", + "detect_changes -- risk-scored impact analysis", + ] + return result + except Exception as exc: + return _error_response(str(exc)) + + +# --------------------------------------------------------------------------- +# Tool: generate_report +# --------------------------------------------------------------------------- + + +def _load_report_template() -> str: + """Load ``report-template.html`` from the installed package assets. + + Falls back to the bundled template string when the asset cannot be + loaded, so the tool never fails solely because the package data is + missing. + """ + if _HAS_IMPORTLIB_RESOURCES: + try: + data = _pkg_files("code_review_graph").joinpath( + "assets/report-template.html" + ).read_text(encoding="utf-8") + if data: + return data + except (FileNotFoundError, OSError): + pass + return _FALLBACK_TEMPLATE + + +# Minimal self-contained template (used if the asset file is unavailable). +_FALLBACK_TEMPLATE = """ + + + +Code Review Report + + + +
+ + + +""" + + +def generate_report_func( + review_data: dict[str, Any], + output_path: str | None = None, + repo_root: str | None = None, +) -> dict[str, Any]: + """Generate a self-contained HTML code review report. + + Injects ``review_data`` (the output of ``score_review_tool`` plus + ``dedupe_findings_tool`` results and free-form verdict/tier/scope) into + the bundled ``report-template.html`` as ``{{REPORT_DATA}}`` and writes + the standalone file (default ``repo_root/code-review-report.html``). + + Args: + review_data: Review data dict (metrics, findings, verdict, tier, + scope, files, baseline, timestamp, manual_review). + output_path: Output file path. Defaults to + ``/code-review-report.html``. + repo_root: Repository root. Auto-detected if omitted. + """ + store, root = _get_store(repo_root) + try: + data = build_report_data(review_data) + data["summary"] = review_data.get("summary", "") + + template = _load_report_template() + rendered = template.replace( + "{{REPORT_DATA}}", json.dumps(data, ensure_ascii=False) + ) + + if output_path: + out = Path(output_path) + if not out.is_absolute(): + out = root / out + else: + out = root / "code-review-report.html" + + out.parent.mkdir(parents=True, exist_ok=True) + out.write_text(rendered, encoding="utf-8") + return { + "status": "ok", + "summary": f"Report written to {out}", + "output_path": str(out), + "report_size_bytes": len(rendered), + } + except Exception as exc: + return _error_response(str(exc)) + finally: + store.close() diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 9f8dc93..4b5dc20 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -21,6 +21,13 @@ Review a PR or branch diff. - Full impact analysis across all PR commits - Structured output with risk assessment +### `/code-review-graph:unified-review` +Three-layer unified code review (CRG graph context + ai-code-review scoring + gstack-review workflow). +- Read-only: every finding waits for a manual fix decision +- Objective Layer-2 metrics via `score_review_tool` +- Fingerprint merge + quality score via `dedupe_findings_tool` +- Standalone HTML report via `generate_report_tool` + ## MCP Tools ### Core Tools @@ -223,6 +230,41 @@ detail_level: str = "standard" Primary tool for code review. Maps changed files to affected functions, flows, communities, and test coverage gaps. Returns risk scores and prioritized review items. Relevant responses may include compact estimated `context_savings` metadata. +### Unified Review Tools + +#### `score_review_tool` +``` +changed_files: list[str] | None # Auto-detected from git diff if omitted +base: str = "HEAD~1" +include_churn: bool = True +repo_root: str | None +detail_level: str = "standard" # "minimal" for grades + values only +``` +Computes objective Layer-2 metrics for changed files: `sql_risk`, +`exception_coverage`, `redundancy_rate`, `high_risk_density`, +`vulnerability_risk`. Each metric carries a `good`/`warn`/`fail` grade, +thresholds and evidence. LLM-judged metrics are listed in `llm_judged`. + +#### `dedupe_findings_tool` +``` +findings: list # [{path, category, severity, confidence, source?}] +suppress_prior: list | None # Previously user-skipped findings to suppress +repo_root: str | None +``` +Merges findings by `path:line:category` fingerprint (highest confidence +wins), boosts multi-source confidence (+1, cap 10), routes low-confidence +findings to the appendix, and computes `PR quality score = max(0, 10 - +(critical*2 + informational*0.5))`. + +#### `generate_report_tool` +``` +review_data: dict # metrics + findings + verdict + tier + scope +output_path: str | None # Default: /code-review-report.html +repo_root: str | None +``` +Renders the standalone HTML code review report (self-contained, no external +dependencies) from the bundled `report-template.html`. + #### `refactor_tool` ``` mode: str = "rename" # "rename", "dead_code", or "suggest" diff --git a/docs/architecture.md b/docs/architecture.md index 6858d9d..c3cf056 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -27,6 +27,8 @@ │ │ ├── Communities: list, get, architecture │ │ │ │ ├── Analysis: detect_changes, refactor, │ │ │ │ │ apply_refactor, hotspots, gaps │ │ +│ │ ├── Scoring: score_review, dedupe_ │ │ +│ │ │ findings, generate_report │ │ │ │ ├── Wiki: generate, get_page │ │ │ │ └── Multi-repo: list_repos, cross_search │ │ │ └────────────────┬───────────────────────────┘ │ @@ -34,16 +36,21 @@ │ ┌───────────┼───────────────┐ ▼ ▼ ▼ - ┌─────────┐ ┌─────────┐ ┌─────────────┐ - │ Parser │ │ Graph │ │ Incremental │ - │ │ │ Store │ │ Engine │ - └────┬────┘ └────┬────┘ └──────┬──────┘ - │ │ │ - ▼ ▼ ▼ - Tree-sitter SQLite DB git/svn diff - grammars (.code-review- subprocess - graph/ - graph.db) + ┌─────────┐ ┌─────────┐ ┌─────────────┐ + │ Parser │ │ Graph │ │ Incremental │ + │ │ │ Store │ │ Engine │ + └────┬────┘ └────┬────┘ └──────┬──────┘ + │ │ │ + ▼ ▼ ▼ + Tree-sitter SQLite DB git/svn diff + grammars (.code-review- subprocess + graph/ + graph.db) + + Scoring module (code_review_graph/scoring.py) sits on top of the + GraphStore and reads changed files + git history to compute the + objective review metrics consumed by the unified-review skill and the + score_review / dedupe_findings / generate_report MCP tools. ``` ## Data Flow diff --git a/skills/unified-review/SKILL.md b/skills/unified-review/SKILL.md new file mode 100644 index 0000000..3d38111 --- /dev/null +++ b/skills/unified-review/SKILL.md @@ -0,0 +1,69 @@ +--- +name: unified-review +description: Three-layer unified code review fusing CRG graph context with ai-code-review scoring methodology and gstack-review fix-first workflow +--- + +# Unified Review + +Perform a three-layer, read-only code review that fuses: + +- **CRG graph context** (blast radius, test gaps, affected flows) +- **ai-code-review methodology** (Layer-1 chain decomposition, Layer-2 quantitative scoring, Layer-3 acceptance) +- **gstack-review workflow** (confidence calibration, fix-first, specialist subagents, review-log persistence) + +**This skill is READ-ONLY.** Every finding is presented to the user for a manual fix decision. Never apply code changes, commit, or push. + +## Token Efficiency Rules +- ALWAYS start with `get_minimal_context(task="unified review")`. Use `detail_level="minimal"` on all calls; escalate to `"standard"` only when a metric or finding needs evidence. + +## Step 0 - Scope and tier + +Read `.code-review.yaml` at the repo root (default tier `standard`). Tiers: `fast` (Layer-1 + blockers only), `standard` (all layers), `strict` (full + every blocker/major fix needs per-item user confirmation). Single-invocation overrides: `快速审查` → fast, `严格审查` → strict. +Detect the project language/framework and the review scope (change/file/service/chain level). Declare both in the report header. + +## Step 1 - Graph context (CRG) + +1. Call `build_or_update_graph_tool()` to ensure the graph is current. +2. Call `get_review_context_tool()` for changed files, blast radius, source snippets and review guidance. +3. Call `detect_changes_tool()` for risk-scored change analysis, test gaps and affected flows. + +## Step 2 - Layer 1: Chain decomposition (ai-code-review) + +Inspect the changed code across eight categories: interface, business, data, utility, error handling, security, performance, observability. Mark each `✅ Clean / ⚠️ Issues Found / — N/A`. Apply the gstack CRITICAL categories as a sub-pass: SQL & Data Safety, Race Conditions & Concurrency, LLM Output Trust Boundary, Shell Injection, and Enum & Value Completeness. Enum completeness requires reading code OUTSIDE the diff (Grep for sibling values, then Read each consumer). + +## Step 3 - Layer 2: Quantitative scoring + +Call `score_review_tool()` for the objective metrics (SQL risk, exception coverage, redundancy, high-risk density, vulnerability heuristic). The remaining metrics (requirement coverage, logic alignment, trust boundaries) are judged by you from the requirements doc or a generic baseline; without a requirements doc halve their weight in the verdict. + +## Step 4 - Specialist dispatch (gstack, diff >= 50 lines) + +When the diff has 50+ changed lines, dispatch specialist subagents in parallel via the Agent/task tool, each with a fresh context and its own checklist: testing, maintainability, security, performance, data-migration, api-contract. Security and data-migration always run (insurance). Collect each specialist's JSON findings. + +## Step 5 - Merge and dedupe + +Call `dedupe_findings_tool(findings=)` to merge by fingerprint (`path:line:category`), boost multi-source confidence (+1, cap 10), route low-confidence findings to the appendix, and compute the PR quality score. + +## Step 6 - Manual adjudication (READ-ONLY) + +Present every merged finding with its severity (🔴 blocker / 🟡 major / 🔵 minor), confidence (1-10), file:line and a proposed fix. Group by severity and ask the user per batch: fix / skip / self-fix. 🔴 blockers cannot be batch-skipped. Record skipped findings for prior-review suppression on the next run. **Do not modify code.** + +## Step 7 - Acceptance gate (ai-code-review) + +Any 🔴 blocker → verdict `❌ FAIL` regardless of other scores. Classify each finding as Ready / Needs Fix / Unusable. Verify the change does not deviate from requirements or architecture conventions. + +## Step 8 - Report + +Call `generate_report_tool(review_data=)` to write `code-review-report.html`. Also present the text report inline. + +## Step 9 - Persistence (optional) + +If the `gstack-review-log` binary is available, record the review outcome (status, counts, quality score, per-finding actions). If it is unavailable, skip silently. + +## Output Format + +`Unified Review: N issues (X blocker, Y major, Z minor) — verdict: ✅ PASS / ❌ FAIL`. List each issue with severity, confidence, file:line, problem, and proposed fix. List manual-review items (payment, order, inventory, permission, distributed-lock, data-migration) explicitly. + +## Token Efficiency Rules +- ALWAYS start with `get_minimal_context(task="unified review")` before any other graph tool. +- Use `detail_level="minimal"` on all calls. Only escalate to `"standard"` when minimal is insufficient. +- Target: complete a unified review in ≤8 tool calls and ≤1200 total output tokens. diff --git a/skills/unified-review/references/common-mistakes.md b/skills/unified-review/references/common-mistakes.md new file mode 100644 index 0000000..52ee84b --- /dev/null +++ b/skills/unified-review/references/common-mistakes.md @@ -0,0 +1,19 @@ +# Unified Review — Common Mistakes + +- **Skipping graph context** — always run `get_minimal_context` first; CRG + context is what makes the review token-efficient and blast-radius aware. +- **Rushing to fix** — this skill is READ-ONLY. Present findings, wait for + user decision. Never apply fixes, commit, or push. +- **Ignoring tier** — read `.code-review.yaml`. `fast` skips Layer 2/3; + `strict` requires per-item confirmation for every blocker/major. +- **Judging metrics without evidence** — `score_review_tool` outputs are + heuristics. Cite the evidence, and let the LLM confirm SQL/exception/vuln + findings before presenting them as facts. +- **Missing manual-review modules** — payment, order, inventory, permission, + distributed-lock, data-migration always require a manual review checklist. +- **Forgetting enum completeness reads OUTSIDE the diff** — grep sibling + values, then read each consumer; in-diff review alone is insufficient. +- **Batch-skipping blockers** — 🔴 blockers cannot be batch-skipped; each + needs an explicit user decision. +- **Not producing the report** — always call `generate_report_tool` at the + end and present the text report inline. diff --git a/skills/unified-review/references/manual-review/data-migration.md b/skills/unified-review/references/manual-review/data-migration.md new file mode 100644 index 0000000..39ebe8e --- /dev/null +++ b/skills/unified-review/references/manual-review/data-migration.md @@ -0,0 +1,12 @@ +# Data Migration — Manual Review Checklist + +High-risk module: schema/data migration requires human confirmation. + +- [ ] Migration is idempotent and re-runnable +- [ ] Forward and rollback paths both defined and tested +- [ ] Backfill is batched / resumable for large tables +- [ ] Data type / precision changes do not silently truncate +- [ ] Nullability and default changes safe for existing rows +- [ ] Migration ordering across shards / replicas is consistent +- [ ] Application deploys compatibly with both old and new schema (expand/contract) +- [ ] Irreversible operations are flagged with a documented reason diff --git a/skills/unified-review/references/manual-review/distributed-lock.md b/skills/unified-review/references/manual-review/distributed-lock.md new file mode 100644 index 0000000..b81b7dd --- /dev/null +++ b/skills/unified-review/references/manual-review/distributed-lock.md @@ -0,0 +1,11 @@ +# Distributed Lock — Manual Review Checklist + +High-risk module: distributed-lock changes require architecture confirmation. + +- [ ] Lock has a TTL / expiry — no permanent deadlock after crash +- [ ] Lock release is atomic and ownership-checked (compare-and-delete) +- [ ] Lock scope is correct (key includes the right entity identifiers) +- [ ] Renewal / watchdog exists for long critical sections +- [ ] Locking order is consistent across paths (no lock-ordering deadlock) +- [ ] Fencing tokens / version check prevents stale-holder writes +- [ ] Fail-open vs fail-closed behavior is intentional and documented diff --git a/skills/unified-review/references/manual-review/inventory.md b/skills/unified-review/references/manual-review/inventory.md new file mode 100644 index 0000000..9b416e6 --- /dev/null +++ b/skills/unified-review/references/manual-review/inventory.md @@ -0,0 +1,11 @@ +# Inventory Module — Manual Review Checklist + +High-risk module: stock/inventory changes require human confirmation. + +- [ ] Stock decrement is atomic (conditional UPDATE, not read-then-write) +- [ ] Oversell prevented: `UPDATE ... SET qty = qty - ? WHERE qty >= ?` +- [ ] Reservation vs. deduction semantics are consistent +- [ ] Concurrent orders cannot both reserve the last unit +- [ ] Restock/return increments handled correctly +- [ ] Inventory events are idempotent (retry-safe) +- [ ] Async stock updates propagate to downstream (warehouse, carts) safely diff --git a/skills/unified-review/references/manual-review/order.md b/skills/unified-review/references/manual-review/order.md new file mode 100644 index 0000000..ca79baa --- /dev/null +++ b/skills/unified-review/references/manual-review/order.md @@ -0,0 +1,12 @@ +# Order Module — Manual Review Checklist + +High-risk module: order lifecycle changes require human confirmation. + +- [ ] State machine transitions are atomic (`WHERE status = ?` updates) +- [ ] Cancellation / timeout / expiry paths complete all side effects +- [ ] Order idempotency key prevents duplicate order creation +- [ ] Price/lock snapshot captured at order time, not at payment time +- [ ] Partial fulfillment / split-shipment handled +- [ ] Negative or inconsistent totals impossible +- [ ] Concurrent edits (cart + order) do not corrupt state +- [ ] Audit trail: every status change logged with reason diff --git a/skills/unified-review/references/manual-review/payment.md b/skills/unified-review/references/manual-review/payment.md new file mode 100644 index 0000000..2391446 --- /dev/null +++ b/skills/unified-review/references/manual-review/payment.md @@ -0,0 +1,14 @@ +# Payment Module — Manual Review Checklist + +High-risk module: payment changes require human confirmation for every +blocker/major fix. + +- [ ] Callback idempotency: a duplicated webhook/callback does not double-charge +- [ ] Amounts stored as fixed-point (integers/cents), never floats +- [ ] Currency codes and precision handled correctly +- [ ] Provider signature / HMAC verification on callbacks +- [ ] Refund logic: correct reversal, no double-refund +- [ ] Failure path: payment timeout, declined, retry semantics +- [ ] Transaction boundary spans charge + order-state update +- [ ] Sensitive data (PAN, tokens) never logged or masked on output +- [ ] Ledger/journal entries are append-only and auditable diff --git a/skills/unified-review/references/manual-review/permission.md b/skills/unified-review/references/manual-review/permission.md new file mode 100644 index 0000000..927eb69 --- /dev/null +++ b/skills/unified-review/references/manual-review/permission.md @@ -0,0 +1,11 @@ +# Permission Module — Manual Review Checklist + +High-risk module: authorization changes require product/human confirmation. + +- [ ] Every endpoint/action enforces the intended permission — no default-allow +- [ ] Role hierarchy / scoping (tenant, org, user) is consistent +- [ ] Object-level permissions checked on read AND write +- [ ] Deny-before-allow ordering is safe +- [ ] Permission checks cannot be bypassed via IDs, query params, or bulk ops +- [ ] New permission/role values handled by all consumers (enum completeness) +- [ ] Sensitive actions audited with actor + target diff --git a/skills/unified-review/references/report-template.html b/skills/unified-review/references/report-template.html new file mode 100644 index 0000000..696dadb --- /dev/null +++ b/skills/unified-review/references/report-template.html @@ -0,0 +1,124 @@ + + + + + +Code Review Report + + + +
+ + + diff --git a/skills/unified-review/references/review-checklist.md b/skills/unified-review/references/review-checklist.md new file mode 100644 index 0000000..fdc4406 --- /dev/null +++ b/skills/unified-review/references/review-checklist.md @@ -0,0 +1,82 @@ +# Unified Review — Generic Checklist + +Reference for the Layer-1 chain decomposition and gstack CRITICAL sub-pass. +Load the language-specific checklist when available (`java-spring.md`, +`python-django.md`, `python-fastapi.md`, `node-express.md`, `go-gin.md`, +`csharp-dotnet.md`, `rust.md`, `php-laravel.md`, `ruby-rails.md`); otherwise +use this generic list. + +## Layer 1 — eight categories + +For each changed area mark ✅ Clean / ⚠️ Issues Found / — N/A. + +1. **Interface** — parameter validation, response conventions, HTTP status + codes, rate limiting, API versioning, protocol correctness +2. **Business** — logic aligns with requirements, state machine correctness, + idempotency design, distributed locks +3. **Data** — SQL injection, query performance, index usage, transaction + boundaries, cache invalidation +4. **Utility** — input validity, no side effects, error return values, + date/time timezone handling +5. **Error handling** — exception classification, fallback logic, error + message sanitization, retry with backoff +6. **Security** — AuthN/AuthZ, sensitive data masking, permission control, + CSRF/XSS prevention +7. **Performance** — N+1 queries, caching strategy, connection pooling, + batch operations, blocking in async paths +8. **Observability** — structured logging with correlation IDs, metrics, + health checks + +## gstack CRITICAL sub-pass (highest severity) + +### SQL & Data Safety +- String interpolation in SQL — use parameterized queries +- TOCTOU check-then-set — use atomic `WHERE` + update +- Bypassing model validations for direct DB writes +- N+1 queries — missing eager loading + +### Race Conditions & Concurrency +- Read-check-write without uniqueness constraint / duplicate-key retry +- find-or-create without a unique DB index +- Status transitions not atomic (`WHERE old_status = ? UPDATE ...`) +- Unsafe HTML rendering on user-controlled data + +### LLM Output Trust Boundary +- LLM-generated values (emails, URLs, names) written to DB without format + validation +- Structured tool output accepted without type/shape checks +- LLM-generated URLs fetched without an allowlist (SSRF) +- LLM output stored in knowledge bases without sanitization (stored prompt + injection) + +### Shell Injection +- `subprocess` with `shell=True` AND interpolated command strings +- `os.system()` with variable interpolation +- `eval()`/`exec()` on LLM-generated code without sandboxing + +### Enum & Value Completeness +- New enum/status/tier values: read (not just grep) every consumer that + switches/filters/displays the value +- Check allowlists and `case`/`if-elsif` chains for fall-through + +## Suppressions — do NOT flag +- Harmless redundancy that aids readability +- "Add a comment explaining a threshold" — thresholds drift +- Consistency-only changes +- Anything already addressed in the diff + +## Severity +- 🔴 **blocker** — must fix before merge (injection, secrets, missing + transaction, auth bypass) → verdict FAIL +- 🟡 **major** — should fix before merge (missing validation, missing + fallback, N+1, unmasked data) +- 🔵 **minor** — can optimize later (naming, duplicate code, comments) + +## Confidence calibration +- 9-10 verified by reading specific code +- 7-8 high-confidence pattern match +- 5-6 medium — show with caveat +- 3-4 low — move to appendix +- 1-2 speculation — suppress unless severity would be P0 + +Every finding: `[SEVERITY] (confidence: N/10) file:line — problem → fix`. diff --git a/skills/unified-review/references/specialists/api-contract.md b/skills/unified-review/references/specialists/api-contract.md new file mode 100644 index 0000000..1c9baf4 --- /dev/null +++ b/skills/unified-review/references/specialists/api-contract.md @@ -0,0 +1,13 @@ +# API Contract Specialist + +Focus: API and interface contract changes in the diff. + +- [ ] Breaking changes to public endpoints (paths, params, response shape) +- [ ] Versioning compatibility (deprecations, fallbacks) +- [ ] Request/response validation matches the schema +- [ ] Error response shape is consistent +- [ ] Authentication/authorization behavior unchanged for existing consumers +- [ ] Renamed/moved functions: all callers updated + +Output JSON lines: +`{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"api-contract","summary":"...","fix":"...","source":"api-contract"}` diff --git a/skills/unified-review/references/specialists/data-migration.md b/skills/unified-review/references/specialists/data-migration.md new file mode 100644 index 0000000..b70be3c --- /dev/null +++ b/skills/unified-review/references/specialists/data-migration.md @@ -0,0 +1,15 @@ +# Data Migration Specialist + +Focus: database schema and data migrations in the diff. + +- [ ] Migration idempotent and re-runnable +- [ ] Forward + rollback paths defined +- [ ] Backfill batched / resumable +- [ ] Type/precision changes do not truncate data +- [ ] Nullability/default changes safe for existing rows +- [ ] Application deploy compatible with old + new schema (expand/contract) + +Insurance specialist — always runs, even when silent. + +Output JSON lines: +`{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"data-migration","summary":"...","fix":"...","source":"data-migration"}` diff --git a/skills/unified-review/references/specialists/maintainability.md b/skills/unified-review/references/specialists/maintainability.md new file mode 100644 index 0000000..188f9db --- /dev/null +++ b/skills/unified-review/references/specialists/maintainability.md @@ -0,0 +1,16 @@ +# Maintainability Specialist + +Focus: code quality and maintainability issues. + +- [ ] Dead code / unreachable branches / unused variables +- [ ] Magic numbers → named constants +- [ ] Overcomplicated abstractions (indirection without payoff) +- [ ] Copy-paste blocks that should be shared (only when it aids clarity) +- [ ] Functions too large / doing too much +- [ ] Stale comments contradicting the code + +Suppress: harmless redundancy that aids readability, comment-on-threshold +requests, consistency-only changes. + +Output JSON lines: +`{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"maintainability","summary":"...","fix":"...","source":"maintainability"}` diff --git a/skills/unified-review/references/specialists/performance.md b/skills/unified-review/references/specialists/performance.md new file mode 100644 index 0000000..e95dc73 --- /dev/null +++ b/skills/unified-review/references/specialists/performance.md @@ -0,0 +1,14 @@ +# Performance Specialist + +Focus: performance and resource efficiency in the diff. + +- [ ] N+1 queries — missing eager loading +- [ ] Unindexed lookups in hot loops +- [ ] O(n×m) lookups in views/loops +- [ ] Blocking calls in async paths (sync subprocess, requests, sleep) +- [ ] Connection pool exhaustion, unbounded retries +- [ ] Bundle/asset size regressions (frontend) +- [ ] Redundant recomputation / missing caching + +Output JSON lines: +`{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"performance","summary":"...","fix":"...","source":"performance"}` diff --git a/skills/unified-review/references/specialists/red-team.md b/skills/unified-review/references/specialists/red-team.md new file mode 100644 index 0000000..38746f8 --- /dev/null +++ b/skills/unified-review/references/specialists/red-team.md @@ -0,0 +1,18 @@ +# Red Team Specialist (conditional) + +Focus: find what the primary and specialist reviewers MISSED. Only dispatched +when the diff is large (>200 lines) or a specialist found a critical issue. + +Think like an attacker and a chaos engineer: + +- [ ] Cross-cutting concerns the specialist checklists do not cover +- [ ] Integration boundary failures (service-to-service, module-to-module) +- [ ] Failure modes: what breaks in production under load, restart, partial + failure +- [ ] Silent data corruption paths (wrong results without errors) +- [ ] Error handling that swallows failures +- [ ] Trust boundary violations +- [ ] Race conditions and edge cases the primary review missed + +Be adversarial. No compliments — just the problems. Tag findings with +`"source":"red-team"`. Output `NO FINDINGS` when nothing new is found. diff --git a/skills/unified-review/references/specialists/security.md b/skills/unified-review/references/specialists/security.md new file mode 100644 index 0000000..429c700 --- /dev/null +++ b/skills/unified-review/references/specialists/security.md @@ -0,0 +1,17 @@ +# Security Specialist + +Focus: security vulnerabilities in the diff. + +- [ ] SQL injection (string interpolation, parameterized queries) +- [ ] AuthN/AuthZ bypasses, missing permission checks +- [ ] XSS (unsafe HTML rendering on user data) +- [ ] Sensitive data exposure / missing masking in logs and responses +- [ ] SSRF (fetching user/LLM-controlled URLs without allowlist) +- [ ] Command injection (`shell=True` + interpolation) +- [ ] Hardcoded secrets / credentials +- [ ] CSRF / missing rate limiting on auth endpoints + +Insurance specialist — always runs, even when silent. + +Output JSON lines: +`{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"security","summary":"...","fix":"...","source":"security"}` diff --git a/skills/unified-review/references/specialists/testing.md b/skills/unified-review/references/specialists/testing.md new file mode 100644 index 0000000..e1e3c84 --- /dev/null +++ b/skills/unified-review/references/specialists/testing.md @@ -0,0 +1,14 @@ +# Testing Specialist + +Focus: test coverage gaps and tests that would catch the issues found. + +- [ ] Every changed function has at least a happy-path test +- [ ] Negative/error paths tested (invalid input, failure branches) +- [ ] Edge cases mirror the happy-path structure +- [ ] If the fix for a finding can be caught by a test, propose a minimal + `test_stub` (framework-detected: jest/vitest/rspec/pytest/go-test) +- [ ] Integration coverage for critical flows (DB, external calls) +- [ ] No assertion-only tests that pass trivially + +Output JSON lines: +`{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"testing","summary":"...","fix":"...","test_stub":"...","source":"testing"}` diff --git a/tests/test_report.py b/tests/test_report.py new file mode 100644 index 0000000..c40e277 --- /dev/null +++ b/tests/test_report.py @@ -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("") + + +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")) diff --git a/tests/test_scoring.py b/tests/test_scoring.py new file mode 100644 index 0000000..7af49da --- /dev/null +++ b/tests/test_scoring.py @@ -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 diff --git a/tests/test_skills.py b/tests/test_skills.py index dd54eca..dfa6f07 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -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: diff --git a/tests/test_unified_review.py b/tests/test_unified_review.py new file mode 100644 index 0000000..4667671 --- /dev/null +++ b/tests/test_unified_review.py @@ -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()