From 232311449db3abff921bbd675c20b7d9dec5666f Mon Sep 17 00:00:00 2001 From: Small-fish-QAQ Date: Mon, 6 Jul 2026 09:12:16 +0800 Subject: [PATCH 1/5] feat: add code review agent MVP example --- examples/skills_code_review_agent/.gitignore | 7 + examples/skills_code_review_agent/README.md | 51 +++++ .../agent/__init__.py | 2 + .../agent/diff_parser.py | 132 ++++++++++++ .../agent/findings.py | 24 +++ .../agent/redaction.py | 47 +++++ .../skills_code_review_agent/agent/report.py | 111 ++++++++++ .../skills_code_review_agent/agent/rules.py | 195 ++++++++++++++++++ .../fixtures/clean.diff | 25 +++ .../fixtures/security.diff | 37 ++++ .../output_samples/review_report.json | 109 ++++++++++ .../output_samples/review_report.md | 24 +++ .../skills_code_review_agent/run_agent.py | 86 ++++++++ 13 files changed, 850 insertions(+) create mode 100644 examples/skills_code_review_agent/.gitignore create mode 100644 examples/skills_code_review_agent/README.md create mode 100644 examples/skills_code_review_agent/agent/__init__.py create mode 100644 examples/skills_code_review_agent/agent/diff_parser.py create mode 100644 examples/skills_code_review_agent/agent/findings.py create mode 100644 examples/skills_code_review_agent/agent/redaction.py create mode 100644 examples/skills_code_review_agent/agent/report.py create mode 100644 examples/skills_code_review_agent/agent/rules.py create mode 100644 examples/skills_code_review_agent/fixtures/clean.diff create mode 100644 examples/skills_code_review_agent/fixtures/security.diff create mode 100644 examples/skills_code_review_agent/output_samples/review_report.json create mode 100644 examples/skills_code_review_agent/output_samples/review_report.md create mode 100644 examples/skills_code_review_agent/run_agent.py diff --git a/examples/skills_code_review_agent/.gitignore b/examples/skills_code_review_agent/.gitignore new file mode 100644 index 00000000..03291b10 --- /dev/null +++ b/examples/skills_code_review_agent/.gitignore @@ -0,0 +1,7 @@ +output/ +output_clean/ +*.db +*.sqlite +*.sqlite3 +__pycache__/ +.pytest_cache/ diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md new file mode 100644 index 00000000..cdd7c631 --- /dev/null +++ b/examples/skills_code_review_agent/README.md @@ -0,0 +1,51 @@ +# Skills Code Review Agent + +This example implements the first phase of issue #92 as a deterministic, +local-only code review loop. It reads a unified diff, scans added lines with a +small static rule set, redacts likely secrets, and writes JSON and Markdown +reports. + +It intentionally does not call an LLM, remote sandbox, or SDK core extension. + +## Run + +From the repository root: + +```bash +python examples/skills_code_review_agent/run_agent.py --diff-file examples/skills_code_review_agent/fixtures/security.diff --output-dir examples/skills_code_review_agent/output --dry-run +``` + +Expected output files: + +```text +examples/skills_code_review_agent/output/ + review_report.json + review_report.md +``` + +You can also run the clean fixture: + +```bash +python examples/skills_code_review_agent/run_agent.py --diff-file examples/skills_code_review_agent/fixtures/clean.diff --output-dir examples/skills_code_review_agent/output_clean --dry-run +``` + +## Current Scope + +- Parses unified diff hunks and added line numbers. +- Runs five deterministic rules: + - hardcoded secret / token / password + - SQL string concatenation risk + - `requests` / `httpx` calls missing `timeout=` + - broad `except Exception` or simple error swallowing + - `open(...)` without `with` +- Redacts likely API keys, tokens, secrets, and passwords before writing reports. +- Produces `review_report.json` and `review_report.md`. + +## Not Implemented Yet + +- Real LLM review. +- Real remote sandbox or container execution. +- SDK core integration. +- SQLite storage. +- Filter governance and telemetry. +- Multi-agent orchestration. diff --git a/examples/skills_code_review_agent/agent/__init__.py b/examples/skills_code_review_agent/agent/__init__.py new file mode 100644 index 00000000..188818cb --- /dev/null +++ b/examples/skills_code_review_agent/agent/__init__.py @@ -0,0 +1,2 @@ +"""Phase-1 deterministic code review example helpers.""" + diff --git a/examples/skills_code_review_agent/agent/diff_parser.py b/examples/skills_code_review_agent/agent/diff_parser.py new file mode 100644 index 00000000..6ca3cc29 --- /dev/null +++ b/examples/skills_code_review_agent/agent/diff_parser.py @@ -0,0 +1,132 @@ +"""Minimal unified diff parser for deterministic review fixtures.""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from typing import Iterable + + +_HUNK_RE = re.compile( + r"^@@ -(?P\d+)(?:,(?P\d+))? \+(?P\d+)(?:,(?P\d+))? @@(?P.*)$" +) + + +@dataclass(frozen=True) +class ChangedLine: + """A line changed by a unified diff hunk.""" + + file_path: str + line_number: int + content: str + hunk_header: str + change_type: str + + +@dataclass(frozen=True) +class ParsedDiff: + """Parsed unified diff data used by the static rule scanner.""" + + files: list[str] + changed_lines: list[ChangedLine] + + +def _normalize_diff_path(raw: str) -> str: + value = raw.strip() + if not value or value == "/dev/null": + return value + path = value.split("\t", maxsplit=1)[0].split(" ", maxsplit=1)[0] + if path.startswith("a/") or path.startswith("b/"): + return path[2:] + return path + + +def _unique_in_order(values: Iterable[str]) -> list[str]: + seen: set[str] = set() + out: list[str] = [] + for value in values: + if value and value not in seen: + seen.add(value) + out.append(value) + return out + + +def parse_unified_diff(diff_text: str) -> ParsedDiff: + """Parse file paths, hunk headers, and changed lines from unified diff text. + + The parser records added and removed lines, but phase-1 rules only scan + additions. Context lines are used only to keep line counters accurate. + """ + + files: list[str] = [] + changed_lines: list[ChangedLine] = [] + + current_file = "" + pending_file = "" + hunk_header = "" + old_line = 0 + new_line = 0 + in_hunk = False + + for raw_line in diff_text.splitlines(): + if raw_line.startswith("diff --git "): + parts = raw_line.split() + if len(parts) >= 4: + pending_file = _normalize_diff_path(parts[3]) + current_file = pending_file + files.append(current_file) + in_hunk = False + hunk_header = "" + continue + + if raw_line.startswith("+++ "): + path = _normalize_diff_path(raw_line[4:]) + if path != "/dev/null": + current_file = path + files.append(current_file) + elif pending_file: + current_file = pending_file + continue + + match = _HUNK_RE.match(raw_line) + if match: + hunk_header = raw_line + old_line = int(match.group("old_start")) + new_line = int(match.group("new_start")) + in_hunk = True + continue + + if not in_hunk or not current_file: + continue + + if raw_line.startswith("+") and not raw_line.startswith("+++"): + changed_lines.append( + ChangedLine( + file_path=current_file, + line_number=new_line, + content=raw_line[1:], + hunk_header=hunk_header, + change_type="add", + ) + ) + new_line += 1 + elif raw_line.startswith("-") and not raw_line.startswith("---"): + changed_lines.append( + ChangedLine( + file_path=current_file, + line_number=old_line, + content=raw_line[1:], + hunk_header=hunk_header, + change_type="delete", + ) + ) + old_line += 1 + elif raw_line.startswith(" "): + old_line += 1 + new_line += 1 + elif raw_line.startswith("\\"): + continue + else: + in_hunk = False + + return ParsedDiff(files=_unique_in_order(files), changed_lines=changed_lines) diff --git a/examples/skills_code_review_agent/agent/findings.py b/examples/skills_code_review_agent/agent/findings.py new file mode 100644 index 00000000..4799993a --- /dev/null +++ b/examples/skills_code_review_agent/agent/findings.py @@ -0,0 +1,24 @@ +"""Finding data model for deterministic code review.""" + +from __future__ import annotations + +from dataclasses import asdict +from dataclasses import dataclass + + +@dataclass(frozen=True) +class Finding: + """A structured code review finding.""" + + severity: str + category: str + file: str + line: int + title: str + evidence: str + recommendation: str + confidence: float + source: str + + def to_dict(self) -> dict[str, object]: + return asdict(self) diff --git a/examples/skills_code_review_agent/agent/redaction.py b/examples/skills_code_review_agent/agent/redaction.py new file mode 100644 index 00000000..7cf8c72f --- /dev/null +++ b/examples/skills_code_review_agent/agent/redaction.py @@ -0,0 +1,47 @@ +"""Redaction helpers for report-safe evidence strings.""" + +from __future__ import annotations + +import hashlib +import re + + +_NAMED_SECRET_RE = re.compile( + r"(?P\b(?:api[_-]?(?:key|token)|access[_-]?token|auth[_-]?token|token|secret|password|passwd|pwd)\b" + r"\s*[:=]\s*[\"']?)(?P[A-Za-z0-9_./+=:@-]{8,})(?P[\"']?)", + re.IGNORECASE, +) +_BEARER_RE = re.compile(r"\bBearer\s+(?P[A-Za-z0-9_./+=-]{12,})", re.IGNORECASE) +_OPENAI_STYLE_RE = re.compile(r"\b(?Psk-[A-Za-z0-9]{12,})\b") + + +def _fingerprint(value: str) -> str: + return hashlib.sha256(value.encode("utf-8")).hexdigest()[:8] + + +def _replacement(value: str) -> str: + return f"" + + +def redact_text(text: str) -> str: + """Redact likely API keys, tokens, secrets, and passwords.""" + + if not text: + return text + + def replace_named(match: re.Match[str]) -> str: + value = match.group("value") + return f"{match.group('prefix')}{_replacement(value)}{match.group('suffix')}" + + def replace_bearer(match: re.Match[str]) -> str: + value = match.group("value") + return f"Bearer {_replacement(value)}" + + def replace_openai_style(match: re.Match[str]) -> str: + value = match.group("value") + return _replacement(value) + + redacted = _NAMED_SECRET_RE.sub(replace_named, text) + redacted = _BEARER_RE.sub(replace_bearer, redacted) + redacted = _OPENAI_STYLE_RE.sub(replace_openai_style, redacted) + return redacted diff --git a/examples/skills_code_review_agent/agent/report.py b/examples/skills_code_review_agent/agent/report.py new file mode 100644 index 00000000..ae2ad613 --- /dev/null +++ b/examples/skills_code_review_agent/agent/report.py @@ -0,0 +1,111 @@ +"""JSON and Markdown report generation.""" + +from __future__ import annotations + +import json +from collections import Counter +from datetime import datetime +from pathlib import Path +from typing import Any + +from .findings import Finding +from .redaction import redact_text + + +_RULES = [ + "static-rule:hardcoded-secret", + "static-rule:sql-string-concat", + "static-rule:http-timeout", + "static-rule:broad-except", + "static-rule:open-without-context-manager", +] + + +def _redact_value(value: Any) -> Any: + if isinstance(value, str): + return redact_text(value) + if isinstance(value, list): + return [_redact_value(item) for item in value] + if isinstance(value, dict): + return {key: _redact_value(item) for key, item in value.items()} + return value + + +def _counts(items: list[str]) -> dict[str, int]: + return dict(sorted(Counter(items).items())) + + +def build_report(*, diff_file: str, files: list[str], findings: list[Finding], dry_run: bool) -> dict[str, Any]: + finding_dicts = [_redact_value(finding.to_dict()) for finding in findings] + severity_counts = _counts([str(item["severity"]) for item in finding_dicts]) + category_counts = _counts([str(item["category"]) for item in finding_dicts]) + + report = { + "summary": { + "generated_at": datetime.utcnow().replace(microsecond=0).isoformat() + "Z", + "dry_run": dry_run, + "diff_file": diff_file, + "files_scanned": files, + "rules": _RULES, + "total_findings": len(finding_dicts), + "severity_counts": severity_counts, + "category_counts": category_counts, + }, + "findings": finding_dicts, + } + return _redact_value(report) + + +def _markdown_table_row(finding: dict[str, Any]) -> str: + evidence = str(finding["evidence"]).replace("|", "\\|") + recommendation = str(finding["recommendation"]).replace("|", "\\|") + return ( + f"| {finding['severity']} | {finding['category']} | " + f"{finding['file']}:{finding['line']} | {finding['title']} | " + f"`{evidence}` | {recommendation} |" + ) + + +def render_markdown(report: dict[str, Any]) -> str: + summary = report["summary"] + lines = [ + "# Code Review Report", + "", + f"- Generated: {summary['generated_at']}", + f"- Dry run: {summary['dry_run']}", + f"- Diff file: `{summary['diff_file']}`", + f"- Files scanned: {len(summary['files_scanned'])}", + f"- Total findings: {summary['total_findings']}", + "", + "## Severity Counts", + "", + ] + if summary["severity_counts"]: + for severity, count in summary["severity_counts"].items(): + lines.append(f"- {severity}: {count}") + else: + lines.append("- none: 0") + + lines.extend(["", "## Findings", ""]) + findings = report["findings"] + if not findings: + lines.append("No findings.") + return "\n".join(lines) + "\n" + + lines.append( + "| Severity | Category | Location | Title | Evidence | Recommendation |" + ) + lines.append("| --- | --- | --- | --- | --- | --- |") + for finding in findings: + lines.append(_markdown_table_row(finding)) + return "\n".join(lines) + "\n" + + +def write_reports(report: dict[str, Any], output_dir: Path) -> tuple[Path, Path]: + output_dir.mkdir(parents=True, exist_ok=True) + json_path = output_dir / "review_report.json" + md_path = output_dir / "review_report.md" + + json_path.write_text(json.dumps(report, indent=2, ensure_ascii=False) + "\n", encoding="utf-8") + md_path.write_text(render_markdown(report), encoding="utf-8") + return json_path, md_path diff --git a/examples/skills_code_review_agent/agent/rules.py b/examples/skills_code_review_agent/agent/rules.py new file mode 100644 index 00000000..40dc8e62 --- /dev/null +++ b/examples/skills_code_review_agent/agent/rules.py @@ -0,0 +1,195 @@ +"""Deterministic static rules for issue #92 phase 1.""" + +from __future__ import annotations + +import re +from collections import defaultdict +from typing import Iterable + +from .diff_parser import ChangedLine +from .findings import Finding +from .redaction import redact_text + + +_SECRET_NAME_RE = re.compile( + r"\b(api[_-]?(?:key|token)|access[_-]?token|auth[_-]?token|token|secret|password|passwd|pwd)\b", + re.I, +) +_SECRET_ASSIGN_RE = re.compile( + r"\b(?:api[_-]?(?:key|token)|access[_-]?token|auth[_-]?token|token|secret|password|passwd|pwd)\b" + r"\s*[:=]\s*[\"'][^\"']{8,}[\"']", + re.I, +) +_SECRET_LITERAL_RE = re.compile(r"\bsk-[A-Za-z0-9]{12,}\b") +_SQL_WORD_RE = re.compile(r"\b(SELECT|INSERT|UPDATE|DELETE|REPLACE|DROP|ALTER)\b", re.I) +_HTTP_CALL_RE = re.compile(r"\b(?:requests|httpx)\.(?:get|post|put|patch|delete|request|stream)\s*\(") +_BROAD_EXCEPT_RE = re.compile(r"^\s*except\s+Exception(?:\s+as\s+\w+)?\s*:") +_OPEN_CALL_RE = re.compile(r"\bopen\s*\(") + + +def _finding( + *, + line: ChangedLine, + severity: str, + category: str, + title: str, + recommendation: str, + confidence: float, + source: str, +) -> Finding: + return Finding( + severity=severity, + category=category, + file=line.file_path, + line=line.line_number, + title=title, + evidence=redact_text(line.content.strip()), + recommendation=recommendation, + confidence=confidence, + source=source, + ) + + +def _added_lines(changed_lines: Iterable[ChangedLine]) -> list[ChangedLine]: + return [line for line in changed_lines if line.change_type == "add"] + + +def _line_map(lines: list[ChangedLine]) -> dict[str, list[ChangedLine]]: + grouped: dict[str, list[ChangedLine]] = defaultdict(list) + for line in lines: + grouped[line.file_path].append(line) + for file_lines in grouped.values(): + file_lines.sort(key=lambda item: item.line_number) + return grouped + + +def _looks_like_secret(line: str) -> bool: + if "os.environ" in line or "getenv(" in line: + return False + return bool(_SECRET_ASSIGN_RE.search(line) or _SECRET_LITERAL_RE.search(line)) + + +def _looks_like_sql_concat(line: str) -> bool: + if not _SQL_WORD_RE.search(line): + return False + compact = line.replace(" ", "") + return any( + marker in line or marker in compact + for marker in ( + " + ", + "+", + "f\"", + "f'", + ".format(", + "% ", + "%(", + ) + ) + + +def _looks_like_missing_timeout(line: str) -> bool: + if not _HTTP_CALL_RE.search(line): + return False + if ")" not in line: + return False + return "timeout=" not in line + + +def _looks_like_resource_lifecycle(line: str) -> bool: + stripped = line.strip() + if stripped.startswith("with "): + return False + if "Path(" in line and ".open(" in line: + return not stripped.startswith("with ") + return bool(_OPEN_CALL_RE.search(line)) + + +def _next_added_line(file_lines: list[ChangedLine], index: int) -> ChangedLine | None: + if index + 1 >= len(file_lines): + return None + return file_lines[index + 1] + + +def run_static_rules(changed_lines: Iterable[ChangedLine]) -> list[Finding]: + """Run all deterministic phase-1 rules over added diff lines.""" + + additions = _added_lines(changed_lines) + findings: list[Finding] = [] + + for line in additions: + content = line.content + + if _looks_like_secret(content): + title = "Possible hardcoded secret" + if _SECRET_NAME_RE.search(content): + title = "Possible hardcoded secret, token, or password" + findings.append( + _finding( + line=line, + severity="high", + category="secret", + title=title, + recommendation="Move secrets to a secret manager or environment variable and rotate exposed values.", + confidence=0.9, + source="static-rule:hardcoded-secret", + ) + ) + + if _looks_like_sql_concat(content): + findings.append( + _finding( + line=line, + severity="high", + category="sql-injection", + title="SQL query appears to be built with string interpolation or concatenation", + recommendation="Use parameterized queries or the database driver's bind parameter API.", + confidence=0.78, + source="static-rule:sql-string-concat", + ) + ) + + if _looks_like_missing_timeout(content): + findings.append( + _finding( + line=line, + severity="medium", + category="network-timeout", + title="HTTP request is missing an explicit timeout", + recommendation="Pass a bounded timeout, for example timeout=10, to avoid hanging workers.", + confidence=0.86, + source="static-rule:http-timeout", + ) + ) + + if _looks_like_resource_lifecycle(content): + findings.append( + _finding( + line=line, + severity="medium", + category="resource-lifecycle", + title="File handle may not be closed on all paths", + recommendation="Use a context manager such as with open(...) as f to guarantee cleanup.", + confidence=0.72, + source="static-rule:open-without-context-manager", + ) + ) + + for file_lines in _line_map(additions).values(): + for index, line in enumerate(file_lines): + if not _BROAD_EXCEPT_RE.search(line.content): + continue + next_line = _next_added_line(file_lines, index) + swallowed = bool(next_line and next_line.content.strip() in {"pass", "return None", "return False"}) + findings.append( + _finding( + line=line, + severity="high" if swallowed else "medium", + category="error-handling", + title="Broad exception handler may hide failures", + recommendation="Catch the narrowest expected exception and log or re-raise unexpected failures.", + confidence=0.82 if swallowed else 0.74, + source="static-rule:broad-except", + ) + ) + + return sorted(findings, key=lambda item: (item.file, item.line, item.category, item.source)) diff --git a/examples/skills_code_review_agent/fixtures/clean.diff b/examples/skills_code_review_agent/fixtures/clean.diff new file mode 100644 index 00000000..45e6545c --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/clean.diff @@ -0,0 +1,25 @@ +diff --git a/app/service.py b/app/service.py +index 1a2b3c4..5d6e7f8 100644 +--- a/app/service.py ++++ b/app/service.py +@@ -1,6 +1,12 @@ + import requests + + + def fetch_user(user_id): +- return {"id": user_id} ++ response = requests.get( ++ "https://example.com/users/" + str(user_id), ++ timeout=10, ++ ) ++ response.raise_for_status() ++ return response.json() + + + def read_config(path): +@@ -8,3 +14,6 @@ def read_config(path): + with open(path, encoding="utf-8") as handle: + return handle.read() ++ ++def build_query(user_id): ++ return "SELECT * FROM users WHERE id = :user_id", {"user_id": user_id} diff --git a/examples/skills_code_review_agent/fixtures/security.diff b/examples/skills_code_review_agent/fixtures/security.diff new file mode 100644 index 00000000..ca7935c1 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/security.diff @@ -0,0 +1,37 @@ +diff --git a/app/payment.py b/app/payment.py +index 1111111..2222222 100644 +--- a/app/payment.py ++++ b/app/payment.py +@@ -1,7 +1,25 @@ + import requests + + + def charge_user(db, user_id): +- return db.execute("SELECT 1") ++ api_token = "sk-live-1234567890abcdef" ++ query = "SELECT * FROM users WHERE id = " + user_id ++ result = db.execute(query) ++ response = requests.post("https://payments.example/charge", json={"user": user_id}) ++ return result, response.json(), api_token ++ ++ ++def load_private_key(path): ++ key_file = open(path, encoding="utf-8") ++ return key_file.read() ++ ++ ++def sync_invoice(client, payload): ++ try: ++ return client.send(payload) ++ except Exception: ++ pass +diff --git a/app/profile.py b/app/profile.py +index 3333333..4444444 100644 +--- a/app/profile.py ++++ b/app/profile.py +@@ -10,5 +10,9 @@ def update_profile(db, name): +- return True ++ password = "prod-password-12345" ++ sql = f"UPDATE users SET name = '{name}'" ++ db.execute(sql) ++ return True diff --git a/examples/skills_code_review_agent/output_samples/review_report.json b/examples/skills_code_review_agent/output_samples/review_report.json new file mode 100644 index 00000000..fff48f4c --- /dev/null +++ b/examples/skills_code_review_agent/output_samples/review_report.json @@ -0,0 +1,109 @@ +{ + "summary": { + "generated_at": "2026-07-05T15:24:57Z", + "dry_run": true, + "diff_file": "examples\\skills_code_review_agent\\fixtures\\security.diff", + "files_scanned": [ + "app/payment.py", + "app/profile.py" + ], + "rules": [ + "static-rule:hardcoded-secret", + "static-rule:sql-string-concat", + "static-rule:http-timeout", + "static-rule:broad-except", + "static-rule:open-without-context-manager" + ], + "total_findings": 7, + "severity_counts": { + "high": 5, + "medium": 2 + }, + "category_counts": { + "error-handling": 1, + "network-timeout": 1, + "resource-lifecycle": 1, + "secret": 2, + "sql-injection": 2 + } + }, + "findings": [ + { + "severity": "high", + "category": "secret", + "file": "app/payment.py", + "line": 5, + "title": "Possible hardcoded secret, token, or password", + "evidence": "api_token = \"\"", + "recommendation": "Move secrets to a secret manager or environment variable and rotate exposed values.", + "confidence": 0.9, + "source": "static-rule:hardcoded-secret" + }, + { + "severity": "high", + "category": "sql-injection", + "file": "app/payment.py", + "line": 6, + "title": "SQL query appears to be built with string interpolation or concatenation", + "evidence": "query = \"SELECT * FROM users WHERE id = \" + user_id", + "recommendation": "Use parameterized queries or the database driver's bind parameter API.", + "confidence": 0.78, + "source": "static-rule:sql-string-concat" + }, + { + "severity": "medium", + "category": "network-timeout", + "file": "app/payment.py", + "line": 8, + "title": "HTTP request is missing an explicit timeout", + "evidence": "response = requests.post(\"https://payments.example/charge\", json={\"user\": user_id})", + "recommendation": "Pass a bounded timeout, for example timeout=10, to avoid hanging workers.", + "confidence": 0.86, + "source": "static-rule:http-timeout" + }, + { + "severity": "medium", + "category": "resource-lifecycle", + "file": "app/payment.py", + "line": 13, + "title": "File handle may not be closed on all paths", + "evidence": "key_file = open(path, encoding=\"utf-8\")", + "recommendation": "Use a context manager such as with open(...) as f to guarantee cleanup.", + "confidence": 0.72, + "source": "static-rule:open-without-context-manager" + }, + { + "severity": "high", + "category": "error-handling", + "file": "app/payment.py", + "line": 20, + "title": "Broad exception handler may hide failures", + "evidence": "except Exception:", + "recommendation": "Catch the narrowest expected exception and log or re-raise unexpected failures.", + "confidence": 0.82, + "source": "static-rule:broad-except" + }, + { + "severity": "high", + "category": "secret", + "file": "app/profile.py", + "line": 10, + "title": "Possible hardcoded secret, token, or password", + "evidence": "password = \"\"", + "recommendation": "Move secrets to a secret manager or environment variable and rotate exposed values.", + "confidence": 0.9, + "source": "static-rule:hardcoded-secret" + }, + { + "severity": "high", + "category": "sql-injection", + "file": "app/profile.py", + "line": 11, + "title": "SQL query appears to be built with string interpolation or concatenation", + "evidence": "sql = f\"UPDATE users SET name = '{name}'\"", + "recommendation": "Use parameterized queries or the database driver's bind parameter API.", + "confidence": 0.78, + "source": "static-rule:sql-string-concat" + } + ] +} diff --git a/examples/skills_code_review_agent/output_samples/review_report.md b/examples/skills_code_review_agent/output_samples/review_report.md new file mode 100644 index 00000000..f7416eee --- /dev/null +++ b/examples/skills_code_review_agent/output_samples/review_report.md @@ -0,0 +1,24 @@ +# Code Review Report + +- Generated: 2026-07-05T15:24:57Z +- Dry run: True +- Diff file: `examples\skills_code_review_agent\fixtures\security.diff` +- Files scanned: 2 +- Total findings: 7 + +## Severity Counts + +- high: 5 +- medium: 2 + +## Findings + +| Severity | Category | Location | Title | Evidence | Recommendation | +| --- | --- | --- | --- | --- | --- | +| high | secret | app/payment.py:5 | Possible hardcoded secret, token, or password | `api_token = ""` | Move secrets to a secret manager or environment variable and rotate exposed values. | +| high | sql-injection | app/payment.py:6 | SQL query appears to be built with string interpolation or concatenation | `query = "SELECT * FROM users WHERE id = " + user_id` | Use parameterized queries or the database driver's bind parameter API. | +| medium | network-timeout | app/payment.py:8 | HTTP request is missing an explicit timeout | `response = requests.post("https://payments.example/charge", json={"user": user_id})` | Pass a bounded timeout, for example timeout=10, to avoid hanging workers. | +| medium | resource-lifecycle | app/payment.py:13 | File handle may not be closed on all paths | `key_file = open(path, encoding="utf-8")` | Use a context manager such as with open(...) as f to guarantee cleanup. | +| high | error-handling | app/payment.py:20 | Broad exception handler may hide failures | `except Exception:` | Catch the narrowest expected exception and log or re-raise unexpected failures. | +| high | secret | app/profile.py:10 | Possible hardcoded secret, token, or password | `password = ""` | Move secrets to a secret manager or environment variable and rotate exposed values. | +| high | sql-injection | app/profile.py:11 | SQL query appears to be built with string interpolation or concatenation | `sql = f"UPDATE users SET name = '{name}'"` | Use parameterized queries or the database driver's bind parameter API. | diff --git a/examples/skills_code_review_agent/run_agent.py b/examples/skills_code_review_agent/run_agent.py new file mode 100644 index 00000000..c03b42e3 --- /dev/null +++ b/examples/skills_code_review_agent/run_agent.py @@ -0,0 +1,86 @@ +#!/usr/bin/env python3 +"""Deterministic dry-run code review example. + +This entrypoint intentionally avoids LLM calls and remote execution. It is a +small, reproducible loop for issue #92 phase 1: + +unified diff -> changed lines -> static rules -> redacted JSON/Markdown report. +""" + +from __future__ import annotations + +import argparse +from pathlib import Path +from typing import Sequence + +from agent.diff_parser import parse_unified_diff +from agent.report import build_report +from agent.report import write_reports +from agent.rules import run_static_rules + + +def _resolve_path(raw: str) -> Path: + path = Path(raw).expanduser() + if path.is_absolute(): + return path + return (Path.cwd() / path).resolve() + + +def _default_diff_path() -> str: + return str(Path(__file__).resolve().parent / "fixtures" / "security.diff") + + +def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: + parser = argparse.ArgumentParser(description="Run deterministic code review over a unified diff.") + parser.add_argument( + "--diff-file", + default=_default_diff_path(), + help="Path to a unified diff file. Defaults to fixtures/security.diff.", + ) + parser.add_argument( + "--output-dir", + default=str(Path(__file__).resolve().parent / "output"), + help="Directory where review_report.json and review_report.md will be written.", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Record the run as dry-run. No external services are called either way.", + ) + return parser.parse_args(argv) + + +def main(argv: Sequence[str] | None = None) -> int: + args = parse_args(argv) + diff_file = _resolve_path(args.diff_file) + output_dir = _resolve_path(args.output_dir) + + if not diff_file.exists(): + raise FileNotFoundError(f"diff file not found: {diff_file}") + + diff_text = diff_file.read_text(encoding="utf-8") + parsed_diff = parse_unified_diff(diff_text) + findings = run_static_rules(parsed_diff.changed_lines) + report = build_report( + diff_file=args.diff_file, + files=parsed_diff.files, + findings=findings, + dry_run=args.dry_run, + ) + json_path, md_path = write_reports(report, output_dir) + + high_count = report["summary"]["severity_counts"].get("high", 0) + medium_count = report["summary"]["severity_counts"].get("medium", 0) + low_count = report["summary"]["severity_counts"].get("low", 0) + + print("Skills code review dry-run complete") + print(f"Diff file: {diff_file}") + print(f"Changed files: {len(parsed_diff.files)}") + print(f"Findings: high={high_count} medium={medium_count} low={low_count}") + print(f"JSON report: {json_path}") + print(f"Markdown report: {md_path}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 4dba7dc593b9d0716ecea406f1c13d7f2709ce32 Mon Sep 17 00:00:00 2001 From: Small-fish-QAQ Date: Mon, 6 Jul 2026 09:25:40 +0800 Subject: [PATCH 2/5] feat: persist code review results with sqlite --- examples/skills_code_review_agent/README.md | 34 +++- .../skills_code_review_agent/agent/storage.py | 145 ++++++++++++++++++ .../skills_code_review_agent/run_agent.py | 18 +++ 3 files changed, 194 insertions(+), 3 deletions(-) create mode 100644 examples/skills_code_review_agent/agent/storage.py diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md index cdd7c631..9fdf2e37 100644 --- a/examples/skills_code_review_agent/README.md +++ b/examples/skills_code_review_agent/README.md @@ -1,9 +1,10 @@ # Skills Code Review Agent -This example implements the first phase of issue #92 as a deterministic, +This example implements the early phases of issue #92 as a deterministic, local-only code review loop. It reads a unified diff, scans added lines with a small static rule set, redacts likely secrets, and writes JSON and Markdown -reports. +reports. When `--db-path` is provided, it also persists the review task, +findings, and report metadata into SQLite. It intentionally does not call an LLM, remote sandbox, or SDK core extension. @@ -29,6 +30,33 @@ You can also run the clean fixture: python examples/skills_code_review_agent/run_agent.py --diff-file examples/skills_code_review_agent/fixtures/clean.diff --output-dir examples/skills_code_review_agent/output_clean --dry-run ``` +## Run With SQLite + +```bash +python examples/skills_code_review_agent/run_agent.py --diff-file examples/skills_code_review_agent/fixtures/security.diff --output-dir examples/skills_code_review_agent/output --db-path examples/skills_code_review_agent/output/reviews.sqlite3 --dry-run +``` + +When `--db-path` is set, the command prints the database path and generated +task id. The database contains three tables: + +- `review_tasks` +- `findings` +- `reports` + +## Verify SQLite Records + +Using Python: + +```bash +python -c "import sqlite3; db='examples/skills_code_review_agent/output/reviews.sqlite3'; con=sqlite3.connect(db); print(con.execute('select task_id,total_findings from review_tasks order by created_at desc limit 1').fetchone()); print(con.execute('select severity,category,file,line,title from findings order by id limit 5').fetchall())" +``` + +Using the `sqlite3` CLI: + +```bash +sqlite3 examples/skills_code_review_agent/output/reviews.sqlite3 "select severity, category, file, line, title from findings order by id;" +``` + ## Current Scope - Parses unified diff hunks and added line numbers. @@ -40,12 +68,12 @@ python examples/skills_code_review_agent/run_agent.py --diff-file examples/skill - `open(...)` without `with` - Redacts likely API keys, tokens, secrets, and passwords before writing reports. - Produces `review_report.json` and `review_report.md`. +- Optionally persists review tasks, findings, and report metadata to SQLite. ## Not Implemented Yet - Real LLM review. - Real remote sandbox or container execution. - SDK core integration. -- SQLite storage. - Filter governance and telemetry. - Multi-agent orchestration. diff --git a/examples/skills_code_review_agent/agent/storage.py b/examples/skills_code_review_agent/agent/storage.py new file mode 100644 index 00000000..bdba7d4d --- /dev/null +++ b/examples/skills_code_review_agent/agent/storage.py @@ -0,0 +1,145 @@ +"""SQLite persistence for deterministic code review runs.""" + +from __future__ import annotations + +import json +import sqlite3 +import uuid +from pathlib import Path +from typing import Any + + +def init_db(db_path: Path) -> None: + """Create the SQLite database and tables if they do not already exist.""" + + db_path.parent.mkdir(parents=True, exist_ok=True) + with sqlite3.connect(db_path) as conn: + conn.execute("PRAGMA foreign_keys = ON") + conn.execute( + """ + CREATE TABLE IF NOT EXISTS review_tasks ( + task_id TEXT PRIMARY KEY, + created_at TEXT NOT NULL, + diff_file TEXT NOT NULL, + dry_run INTEGER NOT NULL, + files_scanned TEXT NOT NULL, + total_findings INTEGER NOT NULL + ) + """ + ) + conn.execute( + """ + CREATE TABLE IF NOT EXISTS findings ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + task_id TEXT NOT NULL, + severity TEXT NOT NULL, + category TEXT NOT NULL, + file TEXT NOT NULL, + line INTEGER NOT NULL, + title TEXT NOT NULL, + evidence TEXT NOT NULL, + recommendation TEXT NOT NULL, + confidence REAL NOT NULL, + source TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) ON DELETE CASCADE + ) + """ + ) + conn.execute( + """ + CREATE TABLE IF NOT EXISTS reports ( + task_id TEXT PRIMARY KEY, + json_report_path TEXT NOT NULL, + markdown_report_path TEXT NOT NULL, + summary_json TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) ON DELETE CASCADE + ) + """ + ) + + +def persist_review( + *, + db_path: Path, + report: dict[str, Any], + json_report_path: Path, + markdown_report_path: Path, +) -> str: + """Persist one review task, its findings, and report metadata.""" + + init_db(db_path) + task_id = str(uuid.uuid4()) + summary = report["summary"] + findings = report["findings"] + + with sqlite3.connect(db_path) as conn: + conn.execute("PRAGMA foreign_keys = ON") + conn.execute( + """ + INSERT INTO review_tasks ( + task_id, + created_at, + diff_file, + dry_run, + files_scanned, + total_findings + ) VALUES (?, ?, ?, ?, ?, ?) + """, + ( + task_id, + summary["generated_at"], + summary["diff_file"], + 1 if summary["dry_run"] else 0, + json.dumps(summary["files_scanned"], ensure_ascii=False), + int(summary["total_findings"]), + ), + ) + conn.executemany( + """ + INSERT INTO findings ( + task_id, + severity, + category, + file, + line, + title, + evidence, + recommendation, + confidence, + source + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + [ + ( + task_id, + finding["severity"], + finding["category"], + finding["file"], + int(finding["line"]), + finding["title"], + finding["evidence"], + finding["recommendation"], + float(finding["confidence"]), + finding["source"], + ) + for finding in findings + ], + ) + conn.execute( + """ + INSERT INTO reports ( + task_id, + json_report_path, + markdown_report_path, + summary_json + ) VALUES (?, ?, ?, ?) + """, + ( + task_id, + str(json_report_path), + str(markdown_report_path), + json.dumps(summary, ensure_ascii=False), + ), + ) + + return task_id diff --git a/examples/skills_code_review_agent/run_agent.py b/examples/skills_code_review_agent/run_agent.py index c03b42e3..7009c139 100644 --- a/examples/skills_code_review_agent/run_agent.py +++ b/examples/skills_code_review_agent/run_agent.py @@ -17,6 +17,7 @@ from agent.report import build_report from agent.report import write_reports from agent.rules import run_static_rules +from agent.storage import persist_review def _resolve_path(raw: str) -> Path: @@ -47,6 +48,11 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: action="store_true", help="Record the run as dry-run. No external services are called either way.", ) + parser.add_argument( + "--db-path", + default="", + help="Optional SQLite database path for persisting review tasks, findings, and reports.", + ) return parser.parse_args(argv) @@ -54,6 +60,7 @@ def main(argv: Sequence[str] | None = None) -> int: args = parse_args(argv) diff_file = _resolve_path(args.diff_file) output_dir = _resolve_path(args.output_dir) + db_path = _resolve_path(args.db_path) if args.db_path else None if not diff_file.exists(): raise FileNotFoundError(f"diff file not found: {diff_file}") @@ -68,6 +75,14 @@ def main(argv: Sequence[str] | None = None) -> int: dry_run=args.dry_run, ) json_path, md_path = write_reports(report, output_dir) + task_id = "" + if db_path is not None: + task_id = persist_review( + db_path=db_path, + report=report, + json_report_path=json_path, + markdown_report_path=md_path, + ) high_count = report["summary"]["severity_counts"].get("high", 0) medium_count = report["summary"]["severity_counts"].get("medium", 0) @@ -79,6 +94,9 @@ def main(argv: Sequence[str] | None = None) -> int: print(f"Findings: high={high_count} medium={medium_count} low={low_count}") print(f"JSON report: {json_path}") print(f"Markdown report: {md_path}") + if db_path is not None: + print(f"Database: {db_path}") + print(f"Task ID: {task_id}") return 0 From 7c6a38c90935c692a5e686c53a3075712ff199d2 Mon Sep 17 00:00:00 2001 From: Small-fish-QAQ Date: Mon, 6 Jul 2026 09:57:29 +0800 Subject: [PATCH 3/5] test: add code review fixtures and pytest coverage --- examples/skills_code_review_agent/README.md | 22 +++++ .../skills_code_review_agent/agent/rules.py | 23 ++++- .../fixtures/broad_except.diff | 11 +++ .../fixtures/duplicate.diff | 12 +++ .../fixtures/missing_timeout.diff | 11 +++ .../fixtures/resource_leak.diff | 9 ++ .../fixtures/secret_redaction.diff | 11 +++ .../fixtures/sql_injection.diff | 9 ++ .../tests/conftest.py | 11 +++ .../tests/test_static_review.py | 86 +++++++++++++++++++ 10 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 examples/skills_code_review_agent/fixtures/broad_except.diff create mode 100644 examples/skills_code_review_agent/fixtures/duplicate.diff create mode 100644 examples/skills_code_review_agent/fixtures/missing_timeout.diff create mode 100644 examples/skills_code_review_agent/fixtures/resource_leak.diff create mode 100644 examples/skills_code_review_agent/fixtures/secret_redaction.diff create mode 100644 examples/skills_code_review_agent/fixtures/sql_injection.diff create mode 100644 examples/skills_code_review_agent/tests/conftest.py create mode 100644 examples/skills_code_review_agent/tests/test_static_review.py diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md index 9fdf2e37..9a8e4018 100644 --- a/examples/skills_code_review_agent/README.md +++ b/examples/skills_code_review_agent/README.md @@ -57,6 +57,27 @@ Using the `sqlite3` CLI: sqlite3 examples/skills_code_review_agent/output/reviews.sqlite3 "select severity, category, file, line, title from findings order by id;" ``` +## Run Tests + +```bash +python -m pytest examples/skills_code_review_agent/tests +``` + +The tests run only local parsing, rules, redaction, report generation, and +dedupe checks. They do not call an LLM, Docker, Cube, remote network, or any +external service. + +## Fixtures + +- `clean.diff`: safe changes; should produce no high-severity findings. +- `security.diff`: mixed security sample covering secret, missing timeout, broad exception, SQL risk, and resource lifecycle. +- `sql_injection.diff`: SQL string concatenation. +- `missing_timeout.diff`: `httpx` request without `timeout=`. +- `broad_except.diff`: broad `except Exception` with swallowed failure. +- `resource_leak.diff`: `open(...)` without a context manager. +- `duplicate.diff`: duplicate hunk producing the same finding, used to verify dedupe. +- `secret_redaction.diff`: multiple secret-like values, used to verify reports omit plaintext secrets. + ## Current Scope - Parses unified diff hunks and added line numbers. @@ -69,6 +90,7 @@ sqlite3 examples/skills_code_review_agent/output/reviews.sqlite3 "select severit - Redacts likely API keys, tokens, secrets, and passwords before writing reports. - Produces `review_report.json` and `review_report.md`. - Optionally persists review tasks, findings, and report metadata to SQLite. +- Includes local pytest coverage for the deterministic rule fixtures. ## Not Implemented Yet diff --git a/examples/skills_code_review_agent/agent/rules.py b/examples/skills_code_review_agent/agent/rules.py index 40dc8e62..9e8d4492 100644 --- a/examples/skills_code_review_agent/agent/rules.py +++ b/examples/skills_code_review_agent/agent/rules.py @@ -110,6 +110,27 @@ def _next_added_line(file_lines: list[ChangedLine], index: int) -> ChangedLine | return file_lines[index + 1] +def _dedupe_findings(findings: list[Finding]) -> list[Finding]: + seen: set[tuple[object, ...]] = set() + deduped: list[Finding] = [] + for finding in findings: + key = ( + finding.severity, + finding.category, + finding.file, + finding.line, + finding.title, + finding.evidence, + finding.recommendation, + finding.source, + ) + if key in seen: + continue + seen.add(key) + deduped.append(finding) + return deduped + + def run_static_rules(changed_lines: Iterable[ChangedLine]) -> list[Finding]: """Run all deterministic phase-1 rules over added diff lines.""" @@ -192,4 +213,4 @@ def run_static_rules(changed_lines: Iterable[ChangedLine]) -> list[Finding]: ) ) - return sorted(findings, key=lambda item: (item.file, item.line, item.category, item.source)) + return sorted(_dedupe_findings(findings), key=lambda item: (item.file, item.line, item.category, item.source)) diff --git a/examples/skills_code_review_agent/fixtures/broad_except.diff b/examples/skills_code_review_agent/fixtures/broad_except.diff new file mode 100644 index 00000000..51f5fb6b --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/broad_except.diff @@ -0,0 +1,11 @@ +diff --git a/app/worker.py b/app/worker.py +index 3000001..3000002 100644 +--- a/app/worker.py ++++ b/app/worker.py +@@ -1,5 +1,10 @@ + def run_job(job): +- return job.run() ++ try: ++ return job.run() ++ except Exception: ++ return None diff --git a/examples/skills_code_review_agent/fixtures/duplicate.diff b/examples/skills_code_review_agent/fixtures/duplicate.diff new file mode 100644 index 00000000..4d4b9a36 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/duplicate.diff @@ -0,0 +1,12 @@ +diff --git a/app/duplicate.py b/app/duplicate.py +index 5000001..5000002 100644 +--- a/app/duplicate.py ++++ b/app/duplicate.py +@@ -1,3 +1,4 @@ + import requests + def fetch(): ++ return requests.get("https://duplicate.example/api") +@@ -1,3 +1,4 @@ + import requests + def fetch(): ++ return requests.get("https://duplicate.example/api") diff --git a/examples/skills_code_review_agent/fixtures/missing_timeout.diff b/examples/skills_code_review_agent/fixtures/missing_timeout.diff new file mode 100644 index 00000000..c11fe3cf --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/missing_timeout.diff @@ -0,0 +1,11 @@ +diff --git a/app/client.py b/app/client.py +index 2000001..2000002 100644 +--- a/app/client.py ++++ b/app/client.py +@@ -1,5 +1,7 @@ + import httpx + + + def load_status(): +- return {"ok": True} ++ return httpx.get("https://status.example/api") diff --git a/examples/skills_code_review_agent/fixtures/resource_leak.diff b/examples/skills_code_review_agent/fixtures/resource_leak.diff new file mode 100644 index 00000000..b622afc9 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/resource_leak.diff @@ -0,0 +1,9 @@ +diff --git a/app/files.py b/app/files.py +index 4000001..4000002 100644 +--- a/app/files.py ++++ b/app/files.py +@@ -1,4 +1,6 @@ + def read_token(path): +- return "" ++ handle = open(path, encoding="utf-8") ++ return handle.read() diff --git a/examples/skills_code_review_agent/fixtures/secret_redaction.diff b/examples/skills_code_review_agent/fixtures/secret_redaction.diff new file mode 100644 index 00000000..e3b06d16 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/secret_redaction.diff @@ -0,0 +1,11 @@ +diff --git a/app/settings.py b/app/settings.py +index 6000001..6000002 100644 +--- a/app/settings.py ++++ b/app/settings.py +@@ -1,4 +1,8 @@ + def settings(): +- return {} ++ api_key = "sk-test-abcdefghijklmnop" ++ auth_token = "token-value-1234567890" ++ password = "correct-horse-prod-password" ++ return {"enabled": True} diff --git a/examples/skills_code_review_agent/fixtures/sql_injection.diff b/examples/skills_code_review_agent/fixtures/sql_injection.diff new file mode 100644 index 00000000..1805f6ad --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/sql_injection.diff @@ -0,0 +1,9 @@ +diff --git a/app/search.py b/app/search.py +index 1000001..1000002 100644 +--- a/app/search.py ++++ b/app/search.py +@@ -1,4 +1,6 @@ + def find_user(db, user_id): +- return db.execute("SELECT * FROM users WHERE id = :id", {"id": user_id}) ++ sql = "SELECT * FROM users WHERE id = " + user_id ++ return db.execute(sql) diff --git a/examples/skills_code_review_agent/tests/conftest.py b/examples/skills_code_review_agent/tests/conftest.py new file mode 100644 index 00000000..2d83fb22 --- /dev/null +++ b/examples/skills_code_review_agent/tests/conftest.py @@ -0,0 +1,11 @@ +"""Pytest setup for the self-contained code review example.""" + +from __future__ import annotations + +import sys +from pathlib import Path + + +EXAMPLE_ROOT = Path(__file__).resolve().parents[1] +if str(EXAMPLE_ROOT) not in sys.path: + sys.path.insert(0, str(EXAMPLE_ROOT)) diff --git a/examples/skills_code_review_agent/tests/test_static_review.py b/examples/skills_code_review_agent/tests/test_static_review.py new file mode 100644 index 00000000..a478b172 --- /dev/null +++ b/examples/skills_code_review_agent/tests/test_static_review.py @@ -0,0 +1,86 @@ +"""Deterministic tests for the phase-1/2 code review example.""" + +from __future__ import annotations + +from pathlib import Path + +from agent.diff_parser import ParsedDiff +from agent.diff_parser import parse_unified_diff +from agent.findings import Finding +from agent.report import build_report +from agent.report import write_reports +from agent.rules import run_static_rules + + +EXAMPLE_ROOT = Path(__file__).resolve().parents[1] +FIXTURES = EXAMPLE_ROOT / "fixtures" + + +def _review_fixture(name: str) -> tuple[ParsedDiff, list[Finding]]: + diff_text = (FIXTURES / name).read_text(encoding="utf-8") + parsed = parse_unified_diff(diff_text) + findings = run_static_rules(parsed.changed_lines) + return parsed, findings + + +def _categories(findings: list[Finding]) -> set[str]: + return {finding.category for finding in findings} + + +def _count_category(findings: list[Finding], category: str) -> int: + return sum(1 for finding in findings if finding.category == category) + + +def test_clean_diff_has_no_high_findings() -> None: + _, findings = _review_fixture("clean.diff") + assert [finding for finding in findings if finding.severity == "high"] == [] + + +def test_security_diff_contains_expected_categories() -> None: + _, findings = _review_fixture("security.diff") + categories = _categories(findings) + assert "secret" in categories + assert "network-timeout" in categories + assert "error-handling" in categories + + +def test_sql_injection_fixture() -> None: + _, findings = _review_fixture("sql_injection.diff") + assert "sql-injection" in _categories(findings) + + +def test_missing_timeout_fixture() -> None: + _, findings = _review_fixture("missing_timeout.diff") + assert "network-timeout" in _categories(findings) + + +def test_broad_except_fixture() -> None: + _, findings = _review_fixture("broad_except.diff") + assert "error-handling" in _categories(findings) + + +def test_resource_leak_fixture() -> None: + _, findings = _review_fixture("resource_leak.diff") + assert "resource-lifecycle" in _categories(findings) + + +def test_secret_redaction_report_omits_plaintext_values(tmp_path: Path) -> None: + parsed, findings = _review_fixture("secret_redaction.diff") + report = build_report( + diff_file="fixtures/secret_redaction.diff", + files=parsed.files, + findings=findings, + dry_run=True, + ) + json_path, md_path = write_reports(report, tmp_path) + combined = json_path.read_text(encoding="utf-8") + md_path.read_text(encoding="utf-8") + + assert "sk-test-abcdefghijklmnop" not in combined + assert "token-value-1234567890" not in combined + assert "correct-horse-prod-password" not in combined + assert " None: + _, findings = _review_fixture("duplicate.diff") + assert _count_category(findings, "network-timeout") == 1 From 41674770baa4a1da5c8aba4438b94c0c4579db94 Mon Sep 17 00:00:00 2001 From: Small-fish-QAQ Date: Mon, 6 Jul 2026 10:43:43 +0800 Subject: [PATCH 4/5] feat: add sandbox filter and telemetry summaries --- examples/skills_code_review_agent/README.md | 31 +++++- .../agent/filtering.py | 56 +++++++++++ .../skills_code_review_agent/agent/report.py | 64 ++++++++++++- .../skills_code_review_agent/agent/sandbox.py | 73 ++++++++++++++ .../skills_code_review_agent/agent/storage.py | 66 +++++++++++++ .../agent/telemetry.py | 35 +++++++ .../output_samples/review_report.json | 38 +++++++- .../skills_code_review_agent/run_agent.py | 24 +++++ .../tests/test_static_review.py | 94 +++++++++++++++++++ 9 files changed, 473 insertions(+), 8 deletions(-) create mode 100644 examples/skills_code_review_agent/agent/filtering.py create mode 100644 examples/skills_code_review_agent/agent/sandbox.py create mode 100644 examples/skills_code_review_agent/agent/telemetry.py diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md index 9a8e4018..3df6d7e4 100644 --- a/examples/skills_code_review_agent/README.md +++ b/examples/skills_code_review_agent/README.md @@ -4,9 +4,12 @@ This example implements the early phases of issue #92 as a deterministic, local-only code review loop. It reads a unified diff, scans added lines with a small static rule set, redacts likely secrets, and writes JSON and Markdown reports. When `--db-path` is provided, it also persists the review task, -findings, and report metadata into SQLite. +findings, report metadata, sandbox run metadata, and filter decisions into +SQLite. It intentionally does not call an LLM, remote sandbox, or SDK core extension. +The sandbox runner in this example is a fake dry-run runner: it records what +would have happened, but never executes untrusted code. ## Run @@ -37,11 +40,13 @@ python examples/skills_code_review_agent/run_agent.py --diff-file examples/skill ``` When `--db-path` is set, the command prints the database path and generated -task id. The database contains three tables: +task id. The database contains these tables: - `review_tasks` - `findings` - `reports` +- `sandbox_runs` +- `filter_decisions` ## Verify SQLite Records @@ -64,8 +69,9 @@ python -m pytest examples/skills_code_review_agent/tests ``` The tests run only local parsing, rules, redaction, report generation, and -dedupe checks. They do not call an LLM, Docker, Cube, remote network, or any -external service. +dedupe checks, fake sandbox status, filter decisions, telemetry summaries, and +SQLite persistence. They do not call an LLM, Docker, Cube, remote network, or +any external service. ## Fixtures @@ -90,12 +96,27 @@ external service. - Redacts likely API keys, tokens, secrets, and passwords before writing reports. - Produces `review_report.json` and `review_report.md`. - Optionally persists review tasks, findings, and report metadata to SQLite. +- Records minimal filter decisions: `allow`, `deny`, or `needs_human_review`. +- Records a fake dry-run sandbox result without executing untrusted code. +- Adds telemetry summary fields to JSON and Markdown reports. - Includes local pytest coverage for the deterministic rule fixtures. +## Sandbox Notes + +The current `dry-run` sandbox runner is deliberately fake and safe for local +development. It simulates a static-check pass and records status/timing fields. +It does not run shell commands, install dependencies, invoke Docker, or reach a +remote sandbox service. + +A true local runner should only be used as a development fallback because it +does not isolate untrusted code. Later phases can replace this example runner +with the project-level `ContainerWorkspaceRuntime` or `CubeWorkspaceRuntime` +without changing the report and storage shape introduced here. + ## Not Implemented Yet - Real LLM review. - Real remote sandbox or container execution. - SDK core integration. -- Filter governance and telemetry. +- Production-grade filter governance and telemetry export. - Multi-agent orchestration. diff --git a/examples/skills_code_review_agent/agent/filtering.py b/examples/skills_code_review_agent/agent/filtering.py new file mode 100644 index 00000000..a3421f60 --- /dev/null +++ b/examples/skills_code_review_agent/agent/filtering.py @@ -0,0 +1,56 @@ +"""Minimal filter decision abstraction for the code review example.""" + +from __future__ import annotations + +import re +from dataclasses import asdict +from dataclasses import dataclass + +from .diff_parser import ParsedDiff + + +_HIGH_RISK_COMMAND_RE = re.compile( + r"\b(" + r"rm\s+-rf\s+/|" + r"curl\b[^|;&]*\|\s*(?:sh|bash)|" + r"wget\b[^|;&]*\|\s*(?:sh|bash)|" + r"chmod\s+777|" + r"Invoke-WebRequest\b[^|;&]*\|\s*iex|" + r"iwr\b[^|;&]*\|\s*iex" + r")", + re.IGNORECASE, +) +_MAX_DIFF_BYTES = 200_000 +_MAX_CHANGED_LINES = 2_000 + + +@dataclass(frozen=True) +class FilterDecision: + """A minimal filter decision for review governance.""" + + decision: str + reason: str + + def to_dict(self) -> dict[str, str]: + return asdict(self) + + +def evaluate_filter_decision(diff_text: str, parsed_diff: ParsedDiff) -> FilterDecision: + """Evaluate whether the review can proceed automatically.""" + + if len(diff_text.encode("utf-8")) > _MAX_DIFF_BYTES: + return FilterDecision( + decision="needs_human_review", + reason=f"diff is larger than {_MAX_DIFF_BYTES} bytes", + ) + if len(parsed_diff.changed_lines) > _MAX_CHANGED_LINES: + return FilterDecision( + decision="needs_human_review", + reason=f"diff changes more than {_MAX_CHANGED_LINES} lines", + ) + if _HIGH_RISK_COMMAND_RE.search(diff_text): + return FilterDecision( + decision="needs_human_review", + reason="diff contains a high-risk command pattern", + ) + return FilterDecision(decision="allow", reason="diff is within dry-run review limits") diff --git a/examples/skills_code_review_agent/agent/report.py b/examples/skills_code_review_agent/agent/report.py index ae2ad613..3037d115 100644 --- a/examples/skills_code_review_agent/agent/report.py +++ b/examples/skills_code_review_agent/agent/report.py @@ -35,10 +35,38 @@ def _counts(items: list[str]) -> dict[str, int]: return dict(sorted(Counter(items).items())) -def build_report(*, diff_file: str, files: list[str], findings: list[Finding], dry_run: bool) -> dict[str, Any]: +def build_report( + *, + diff_file: str, + files: list[str], + findings: list[Finding], + dry_run: bool, + filter_summary: dict[str, Any] | None = None, + sandbox_summary: dict[str, Any] | None = None, + telemetry_summary: dict[str, Any] | None = None, +) -> dict[str, Any]: finding_dicts = [_redact_value(finding.to_dict()) for finding in findings] severity_counts = _counts([str(item["severity"]) for item in finding_dicts]) category_counts = _counts([str(item["category"]) for item in finding_dicts]) + filter_summary = filter_summary or {"decision": "allow", "reason": "not evaluated"} + sandbox_summary = sandbox_summary or { + "runner_name": "none", + "timeout_seconds": 0, + "status": "not_run", + "started_at": "", + "finished_at": "", + "stdout_summary": "", + "stderr_summary": "", + } + telemetry_summary = telemetry_summary or { + "files_scanned": files, + "total_findings": len(finding_dicts), + "severity_counts": severity_counts, + "category_counts": category_counts, + "sandbox_status": sandbox_summary["status"], + "filter_decision": filter_summary["decision"], + "duration_ms": 0, + } report = { "summary": { @@ -51,6 +79,9 @@ def build_report(*, diff_file: str, files: list[str], findings: list[Finding], d "severity_counts": severity_counts, "category_counts": category_counts, }, + "filter": filter_summary, + "sandbox": sandbox_summary, + "telemetry": telemetry_summary, "findings": finding_dicts, } return _redact_value(report) @@ -68,6 +99,9 @@ def _markdown_table_row(finding: dict[str, Any]) -> str: def render_markdown(report: dict[str, Any]) -> str: summary = report["summary"] + filter_summary = report.get("filter", {}) + sandbox_summary = report.get("sandbox", {}) + telemetry_summary = report.get("telemetry", {}) lines = [ "# Code Review Report", "", @@ -76,6 +110,8 @@ def render_markdown(report: dict[str, Any]) -> str: f"- Diff file: `{summary['diff_file']}`", f"- Files scanned: {len(summary['files_scanned'])}", f"- Total findings: {summary['total_findings']}", + f"- Filter decision: {filter_summary.get('decision', 'unknown')}", + f"- Sandbox status: {sandbox_summary.get('status', 'unknown')}", "", "## Severity Counts", "", @@ -86,6 +122,32 @@ def render_markdown(report: dict[str, Any]) -> str: else: lines.append("- none: 0") + lines.extend([ + "", + "## Filter Summary", + "", + f"- Decision: {filter_summary.get('decision', 'unknown')}", + f"- Reason: {filter_summary.get('reason', '')}", + "", + "## Sandbox Summary", + "", + f"- Runner: {sandbox_summary.get('runner_name', 'unknown')}", + f"- Timeout seconds: {sandbox_summary.get('timeout_seconds', 0)}", + f"- Status: {sandbox_summary.get('status', 'unknown')}", + f"- Started: {sandbox_summary.get('started_at', '')}", + f"- Finished: {sandbox_summary.get('finished_at', '')}", + f"- Stdout summary: {sandbox_summary.get('stdout_summary', '')}", + f"- Stderr summary: {sandbox_summary.get('stderr_summary', '')}", + "", + "## Telemetry Summary", + "", + f"- Files scanned: {len(telemetry_summary.get('files_scanned', []))}", + f"- Total findings: {telemetry_summary.get('total_findings', 0)}", + f"- Sandbox status: {telemetry_summary.get('sandbox_status', 'unknown')}", + f"- Filter decision: {telemetry_summary.get('filter_decision', 'unknown')}", + f"- Duration ms: {telemetry_summary.get('duration_ms', 0)}", + ]) + lines.extend(["", "## Findings", ""]) findings = report["findings"] if not findings: diff --git a/examples/skills_code_review_agent/agent/sandbox.py b/examples/skills_code_review_agent/agent/sandbox.py new file mode 100644 index 00000000..d4050f5b --- /dev/null +++ b/examples/skills_code_review_agent/agent/sandbox.py @@ -0,0 +1,73 @@ +"""Minimal dry-run sandbox runner abstraction. + +This module intentionally does not execute untrusted code. It only records a +structured dry-run result so later phases can replace the runner with a real +container or Cube workspace runtime. +""" + +from __future__ import annotations + +from dataclasses import asdict +from dataclasses import dataclass +from datetime import datetime +from typing import Sequence + +from .filtering import FilterDecision +from .findings import Finding + + +def _utc_now() -> str: + return datetime.utcnow().replace(microsecond=0).isoformat() + "Z" + + +@dataclass(frozen=True) +class SandboxRun: + """Structured summary of a sandbox runner invocation.""" + + runner_name: str + timeout_seconds: int + status: str + started_at: str + finished_at: str + stdout_summary: str + stderr_summary: str + + def to_dict(self) -> dict[str, object]: + return asdict(self) + + +class DryRunSandboxRunner: + """A fake sandbox runner that records a completed static-check simulation.""" + + def __init__(self, timeout_seconds: int = 30) -> None: + self.runner_name = "dry-run" + self.timeout_seconds = timeout_seconds + + def run( + self, + *, + files: Sequence[str], + findings: Sequence[Finding], + filter_decision: FilterDecision, + ) -> SandboxRun: + started_at = _utc_now() + finished_at = _utc_now() + if filter_decision.decision == "deny": + return SandboxRun( + runner_name=self.runner_name, + timeout_seconds=self.timeout_seconds, + status="skipped", + started_at=started_at, + finished_at=finished_at, + stdout_summary="dry-run sandbox skipped because filter denied the review", + stderr_summary=filter_decision.reason, + ) + return SandboxRun( + runner_name=self.runner_name, + timeout_seconds=self.timeout_seconds, + status="completed", + started_at=started_at, + finished_at=finished_at, + stdout_summary=f"simulated static checks for {len(files)} files and {len(findings)} findings", + stderr_summary="", + ) diff --git a/examples/skills_code_review_agent/agent/storage.py b/examples/skills_code_review_agent/agent/storage.py index bdba7d4d..a2427687 100644 --- a/examples/skills_code_review_agent/agent/storage.py +++ b/examples/skills_code_review_agent/agent/storage.py @@ -56,6 +56,32 @@ def init_db(db_path: Path) -> None: ) """ ) + conn.execute( + """ + CREATE TABLE IF NOT EXISTS sandbox_runs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + task_id TEXT NOT NULL, + runner_name TEXT NOT NULL, + timeout_seconds INTEGER NOT NULL, + status TEXT NOT NULL, + started_at TEXT NOT NULL, + finished_at TEXT NOT NULL, + stdout_summary TEXT NOT NULL, + stderr_summary TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) ON DELETE CASCADE + ) + """ + ) + conn.execute( + """ + CREATE TABLE IF NOT EXISTS filter_decisions ( + task_id TEXT PRIMARY KEY, + decision TEXT NOT NULL, + reason TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) ON DELETE CASCADE + ) + """ + ) def persist_review( @@ -71,6 +97,8 @@ def persist_review( task_id = str(uuid.uuid4()) summary = report["summary"] findings = report["findings"] + sandbox = report.get("sandbox", {}) + filter_decision = report.get("filter", {}) with sqlite3.connect(db_path) as conn: conn.execute("PRAGMA foreign_keys = ON") @@ -141,5 +169,43 @@ def persist_review( json.dumps(summary, ensure_ascii=False), ), ) + conn.execute( + """ + INSERT INTO sandbox_runs ( + task_id, + runner_name, + timeout_seconds, + status, + started_at, + finished_at, + stdout_summary, + stderr_summary + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + task_id, + sandbox.get("runner_name", ""), + int(sandbox.get("timeout_seconds", 0)), + sandbox.get("status", ""), + sandbox.get("started_at", ""), + sandbox.get("finished_at", ""), + sandbox.get("stdout_summary", ""), + sandbox.get("stderr_summary", ""), + ), + ) + conn.execute( + """ + INSERT INTO filter_decisions ( + task_id, + decision, + reason + ) VALUES (?, ?, ?) + """, + ( + task_id, + filter_decision.get("decision", ""), + filter_decision.get("reason", ""), + ), + ) return task_id diff --git a/examples/skills_code_review_agent/agent/telemetry.py b/examples/skills_code_review_agent/agent/telemetry.py new file mode 100644 index 00000000..ea4bdc13 --- /dev/null +++ b/examples/skills_code_review_agent/agent/telemetry.py @@ -0,0 +1,35 @@ +"""Telemetry summary helpers for the deterministic code review example.""" + +from __future__ import annotations + +from collections import Counter +from typing import Sequence + +from .filtering import FilterDecision +from .findings import Finding +from .sandbox import SandboxRun + + +def _counts(items: list[str]) -> dict[str, int]: + return dict(sorted(Counter(items).items())) + + +def build_telemetry_summary( + *, + files_scanned: Sequence[str], + findings: Sequence[Finding], + sandbox_run: SandboxRun, + filter_decision: FilterDecision, + duration_ms: int, +) -> dict[str, object]: + """Build a small telemetry summary for reports and storage.""" + + return { + "files_scanned": list(files_scanned), + "total_findings": len(findings), + "severity_counts": _counts([finding.severity for finding in findings]), + "category_counts": _counts([finding.category for finding in findings]), + "sandbox_status": sandbox_run.status, + "filter_decision": filter_decision.decision, + "duration_ms": max(0, int(duration_ms)), + } diff --git a/examples/skills_code_review_agent/output_samples/review_report.json b/examples/skills_code_review_agent/output_samples/review_report.json index fff48f4c..a7c721ca 100644 --- a/examples/skills_code_review_agent/output_samples/review_report.json +++ b/examples/skills_code_review_agent/output_samples/review_report.json @@ -1,8 +1,8 @@ { "summary": { - "generated_at": "2026-07-05T15:24:57Z", + "generated_at": "2026-07-06T02:09:48Z", "dry_run": true, - "diff_file": "examples\\skills_code_review_agent\\fixtures\\security.diff", + "diff_file": "examples/skills_code_review_agent/fixtures/security.diff", "files_scanned": [ "app/payment.py", "app/profile.py" @@ -27,6 +27,40 @@ "sql-injection": 2 } }, + "filter": { + "decision": "allow", + "reason": "diff is within dry-run review limits" + }, + "sandbox": { + "runner_name": "dry-run", + "timeout_seconds": 30, + "status": "completed", + "started_at": "2026-07-06T02:09:48Z", + "finished_at": "2026-07-06T02:09:48Z", + "stdout_summary": "simulated static checks for 2 files and 7 findings", + "stderr_summary": "" + }, + "telemetry": { + "files_scanned": [ + "app/payment.py", + "app/profile.py" + ], + "total_findings": 7, + "severity_counts": { + "high": 5, + "medium": 2 + }, + "category_counts": { + "error-handling": 1, + "network-timeout": 1, + "resource-lifecycle": 1, + "secret": 2, + "sql-injection": 2 + }, + "sandbox_status": "completed", + "filter_decision": "allow", + "duration_ms": 16 + }, "findings": [ { "severity": "high", diff --git a/examples/skills_code_review_agent/run_agent.py b/examples/skills_code_review_agent/run_agent.py index 7009c139..a1d9ff02 100644 --- a/examples/skills_code_review_agent/run_agent.py +++ b/examples/skills_code_review_agent/run_agent.py @@ -10,14 +10,18 @@ from __future__ import annotations import argparse +import time from pathlib import Path from typing import Sequence from agent.diff_parser import parse_unified_diff +from agent.filtering import evaluate_filter_decision from agent.report import build_report from agent.report import write_reports from agent.rules import run_static_rules +from agent.sandbox import DryRunSandboxRunner from agent.storage import persist_review +from agent.telemetry import build_telemetry_summary def _resolve_path(raw: str) -> Path: @@ -57,6 +61,7 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: def main(argv: Sequence[str] | None = None) -> int: + started = time.perf_counter() args = parse_args(argv) diff_file = _resolve_path(args.diff_file) output_dir = _resolve_path(args.output_dir) @@ -67,12 +72,29 @@ def main(argv: Sequence[str] | None = None) -> int: diff_text = diff_file.read_text(encoding="utf-8") parsed_diff = parse_unified_diff(diff_text) + filter_decision = evaluate_filter_decision(diff_text, parsed_diff) findings = run_static_rules(parsed_diff.changed_lines) + sandbox_run = DryRunSandboxRunner().run( + files=parsed_diff.files, + findings=findings, + filter_decision=filter_decision, + ) + duration_ms = int((time.perf_counter() - started) * 1000) + telemetry_summary = build_telemetry_summary( + files_scanned=parsed_diff.files, + findings=findings, + sandbox_run=sandbox_run, + filter_decision=filter_decision, + duration_ms=duration_ms, + ) report = build_report( diff_file=args.diff_file, files=parsed_diff.files, findings=findings, dry_run=args.dry_run, + filter_summary=filter_decision.to_dict(), + sandbox_summary=sandbox_run.to_dict(), + telemetry_summary=telemetry_summary, ) json_path, md_path = write_reports(report, output_dir) task_id = "" @@ -92,6 +114,8 @@ def main(argv: Sequence[str] | None = None) -> int: print(f"Diff file: {diff_file}") print(f"Changed files: {len(parsed_diff.files)}") print(f"Findings: high={high_count} medium={medium_count} low={low_count}") + print(f"Filter decision: {filter_decision.decision} ({filter_decision.reason})") + print(f"Sandbox status: {sandbox_run.status}") print(f"JSON report: {json_path}") print(f"Markdown report: {md_path}") if db_path is not None: diff --git a/examples/skills_code_review_agent/tests/test_static_review.py b/examples/skills_code_review_agent/tests/test_static_review.py index a478b172..411e6c11 100644 --- a/examples/skills_code_review_agent/tests/test_static_review.py +++ b/examples/skills_code_review_agent/tests/test_static_review.py @@ -2,14 +2,19 @@ from __future__ import annotations +import sqlite3 from pathlib import Path from agent.diff_parser import ParsedDiff from agent.diff_parser import parse_unified_diff +from agent.filtering import evaluate_filter_decision from agent.findings import Finding from agent.report import build_report from agent.report import write_reports from agent.rules import run_static_rules +from agent.sandbox import DryRunSandboxRunner +from agent.storage import persist_review +from agent.telemetry import build_telemetry_summary EXAMPLE_ROOT = Path(__file__).resolve().parents[1] @@ -23,6 +28,13 @@ def _review_fixture(name: str) -> tuple[ParsedDiff, list[Finding]]: return parsed, findings +def _review_fixture_with_text(name: str) -> tuple[str, ParsedDiff, list[Finding]]: + diff_text = (FIXTURES / name).read_text(encoding="utf-8") + parsed = parse_unified_diff(diff_text) + findings = run_static_rules(parsed.changed_lines) + return diff_text, parsed, findings + + def _categories(findings: list[Finding]) -> set[str]: return {finding.category for finding in findings} @@ -84,3 +96,85 @@ def test_secret_redaction_report_omits_plaintext_values(tmp_path: Path) -> None: def test_duplicate_fixture_dedupes_repeated_finding() -> None: _, findings = _review_fixture("duplicate.diff") assert _count_category(findings, "network-timeout") == 1 + + +def test_default_filter_decision_is_allow() -> None: + diff_text, parsed, _ = _review_fixture_with_text("clean.diff") + decision = evaluate_filter_decision(diff_text, parsed) + assert decision.decision == "allow" + + +def test_sandbox_dry_run_status_is_completed() -> None: + diff_text, parsed, findings = _review_fixture_with_text("security.diff") + decision = evaluate_filter_decision(diff_text, parsed) + sandbox_run = DryRunSandboxRunner().run( + files=parsed.files, + findings=findings, + filter_decision=decision, + ) + assert sandbox_run.runner_name == "dry-run" + assert sandbox_run.status == "completed" + + +def test_telemetry_summary_contains_findings_counts() -> None: + diff_text, parsed, findings = _review_fixture_with_text("security.diff") + decision = evaluate_filter_decision(diff_text, parsed) + sandbox_run = DryRunSandboxRunner().run( + files=parsed.files, + findings=findings, + filter_decision=decision, + ) + telemetry = build_telemetry_summary( + files_scanned=parsed.files, + findings=findings, + sandbox_run=sandbox_run, + filter_decision=decision, + duration_ms=12, + ) + assert telemetry["total_findings"] == len(findings) + assert telemetry["severity_counts"]["high"] >= 1 + + +def test_sqlite_persists_sandbox_and_filter_rows(tmp_path: Path) -> None: + diff_text, parsed, findings = _review_fixture_with_text("security.diff") + decision = evaluate_filter_decision(diff_text, parsed) + sandbox_run = DryRunSandboxRunner().run( + files=parsed.files, + findings=findings, + filter_decision=decision, + ) + telemetry = build_telemetry_summary( + files_scanned=parsed.files, + findings=findings, + sandbox_run=sandbox_run, + filter_decision=decision, + duration_ms=25, + ) + report = build_report( + diff_file="fixtures/security.diff", + files=parsed.files, + findings=findings, + dry_run=True, + filter_summary=decision.to_dict(), + sandbox_summary=sandbox_run.to_dict(), + telemetry_summary=telemetry, + ) + json_path, md_path = write_reports(report, tmp_path) + db_path = tmp_path / "reviews.sqlite3" + task_id = persist_review( + db_path=db_path, + report=report, + json_report_path=json_path, + markdown_report_path=md_path, + ) + + with sqlite3.connect(db_path) as conn: + sandbox_count = conn.execute("select count(*) from sandbox_runs where task_id = ?", (task_id, )).fetchone()[0] + filter_count = conn.execute("select count(*) from filter_decisions where task_id = ?", (task_id, )).fetchone()[0] + sandbox_status = conn.execute("select status from sandbox_runs where task_id = ?", (task_id, )).fetchone()[0] + filter_decision = conn.execute("select decision from filter_decisions where task_id = ?", (task_id, )).fetchone()[0] + + assert sandbox_count == 1 + assert filter_count == 1 + assert sandbox_status == "completed" + assert filter_decision == "allow" From 80bff220a09467c241814e1e444e825071d1f645 Mon Sep 17 00:00:00 2001 From: Small-fish-QAQ Date: Mon, 6 Jul 2026 11:37:09 +0800 Subject: [PATCH 5/5] feat: add lightweight CI gate and stdin diff support --- examples/skills_code_review_agent/README.md | 47 ++++++++++-- .../skills_code_review_agent/agent/rules.py | 38 ++++++++++ .../skills_code_review_agent/run_agent.py | 73 ++++++++++++++++--- .../tests/test_static_review.py | 72 ++++++++++++++++++ 4 files changed, 215 insertions(+), 15 deletions(-) diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md index 3df6d7e4..e40e7ef3 100644 --- a/examples/skills_code_review_agent/README.md +++ b/examples/skills_code_review_agent/README.md @@ -7,9 +7,11 @@ reports. When `--db-path` is provided, it also persists the review task, findings, report metadata, sandbox run metadata, and filter decisions into SQLite. -It intentionally does not call an LLM, remote sandbox, or SDK core extension. -The sandbox runner in this example is a fake dry-run runner: it records what -would have happened, but never executes untrusted code. +This is a lightweight deterministic baseline, not a full sandbox/scanner +implementation. It intentionally does not call an LLM, remote sandbox, Docker, +Cube, E2B, external scanner, or SDK core extension. The sandbox runner in this +example is a fake dry-run runner: it records what would have happened, but +never executes untrusted code. ## Run @@ -33,6 +35,36 @@ You can also run the clean fixture: python examples/skills_code_review_agent/run_agent.py --diff-file examples/skills_code_review_agent/fixtures/clean.diff --output-dir examples/skills_code_review_agent/output_clean --dry-run ``` +## Run From Stdin + +`--diff-file -` reads a unified diff from stdin and records the report input as +``: + +```bash +git diff | python examples/skills_code_review_agent/run_agent.py --diff-file - --output-dir examples/skills_code_review_agent/output --dry-run +``` + +## CI Failure Gate + +By default the command exits with `0` even when findings are present. Use +`--fail-on-severity` to make a lightweight CI gate: + +```bash +python examples/skills_code_review_agent/run_agent.py --diff-file examples/skills_code_review_agent/fixtures/security.diff --output-dir examples/skills_code_review_agent/output --dry-run --fail-on-severity high +``` + +Values are `never`, `low`, `medium`, and `high`. For example, `medium` fails on +medium or high findings, while `high` fails only on high findings. + +## List Rules + +```bash +python examples/skills_code_review_agent/run_agent.py --list-rules +``` + +This prints the deterministic rule id, category, default severity, description, +and known limitations without reading a diff. + ## Run With SQLite ```bash @@ -70,8 +102,9 @@ python -m pytest examples/skills_code_review_agent/tests The tests run only local parsing, rules, redaction, report generation, and dedupe checks, fake sandbox status, filter decisions, telemetry summaries, and -SQLite persistence. They do not call an LLM, Docker, Cube, remote network, or -any external service. +SQLite persistence. They also cover the lightweight CI failure gate, stdin diff +input, and rule listing. They do not call an LLM, Docker, Cube, remote network, +external scanners, or any external service. ## Fixtures @@ -95,6 +128,9 @@ any external service. - `open(...)` without `with` - Redacts likely API keys, tokens, secrets, and passwords before writing reports. - Produces `review_report.json` and `review_report.md`. +- Supports reading a unified diff from stdin with `--diff-file -`. +- Supports a lightweight exit-code gate with `--fail-on-severity`. +- Prints rule metadata with `--list-rules`. - Optionally persists review tasks, findings, and report metadata to SQLite. - Records minimal filter decisions: `allow`, `deny`, or `needs_human_review`. - Records a fake dry-run sandbox result without executing untrusted code. @@ -117,6 +153,7 @@ without changing the report and storage shape introduced here. - Real LLM review. - Real remote sandbox or container execution. +- External scanner integration such as Bandit, Ruff, detect-secrets, or Semgrep. - SDK core integration. - Production-grade filter governance and telemetry export. - Multi-agent orchestration. diff --git a/examples/skills_code_review_agent/agent/rules.py b/examples/skills_code_review_agent/agent/rules.py index 9e8d4492..b89f6e9d 100644 --- a/examples/skills_code_review_agent/agent/rules.py +++ b/examples/skills_code_review_agent/agent/rules.py @@ -26,6 +26,44 @@ _BROAD_EXCEPT_RE = re.compile(r"^\s*except\s+Exception(?:\s+as\s+\w+)?\s*:") _OPEN_CALL_RE = re.compile(r"\bopen\s*\(") +RULES_MANIFEST = [ + { + "id": "static-rule:hardcoded-secret", + "category": "secret", + "default_severity": "high", + "description": "Flags added lines that appear to assign hardcoded API keys, tokens, secrets, or passwords.", + "limitations": "Regex-based and line-oriented; it can miss split secrets and may flag synthetic test data.", + }, + { + "id": "static-rule:sql-string-concat", + "category": "sql-injection", + "default_severity": "high", + "description": "Flags SQL statements that appear to use string interpolation, formatting, or concatenation.", + "limitations": "Does not parse Python AST or validate actual database driver parameter usage.", + }, + { + "id": "static-rule:http-timeout", + "category": "network-timeout", + "default_severity": "medium", + "description": "Flags requests/httpx calls on one added line when no explicit timeout= argument is present.", + "limitations": "Only handles simple single-line calls and cannot resolve wrapper defaults.", + }, + { + "id": "static-rule:broad-except", + "category": "error-handling", + "default_severity": "medium/high", + "description": "Flags broad except Exception handlers, escalating when the next added line swallows the error.", + "limitations": "Line-oriented; it does not build a control-flow graph or inspect existing surrounding code.", + }, + { + "id": "static-rule:open-without-context-manager", + "category": "resource-lifecycle", + "default_severity": "medium", + "description": "Flags simple open(...) usage that is not introduced with a with statement.", + "limitations": "Does not track close() calls across later lines or helper abstractions.", + }, +] + def _finding( *, diff --git a/examples/skills_code_review_agent/run_agent.py b/examples/skills_code_review_agent/run_agent.py index a1d9ff02..98819511 100644 --- a/examples/skills_code_review_agent/run_agent.py +++ b/examples/skills_code_review_agent/run_agent.py @@ -10,6 +10,7 @@ from __future__ import annotations import argparse +import sys import time from pathlib import Path from typing import Sequence @@ -18,12 +19,20 @@ from agent.filtering import evaluate_filter_decision from agent.report import build_report from agent.report import write_reports +from agent.rules import RULES_MANIFEST from agent.rules import run_static_rules from agent.sandbox import DryRunSandboxRunner from agent.storage import persist_review from agent.telemetry import build_telemetry_summary +_SEVERITY_RANK = { + "low": 1, + "medium": 2, + "high": 3, +} + + def _resolve_path(raw: str) -> Path: path = Path(raw).expanduser() if path.is_absolute(): @@ -35,12 +44,39 @@ def _default_diff_path() -> str: return str(Path(__file__).resolve().parent / "fixtures" / "security.diff") +def _read_diff(raw: str) -> tuple[str, str, str]: + if raw == "-": + return "", "", sys.stdin.read() + + diff_file = _resolve_path(raw) + if not diff_file.exists(): + raise FileNotFoundError(f"diff file not found: {diff_file}") + return str(diff_file), raw, diff_file.read_text(encoding="utf-8") + + +def _print_rules() -> None: + print("Deterministic rules") + for rule in RULES_MANIFEST: + print(f"- {rule['id']}") + print(f" category: {rule['category']}") + print(f" default severity: {rule['default_severity']}") + print(f" description: {rule['description']}") + print(f" limitations: {rule['limitations']}") + + +def _failure_gate_triggered(findings: Sequence[object], threshold: str) -> bool: + if threshold == "never": + return False + minimum = _SEVERITY_RANK[threshold] + return any(_SEVERITY_RANK.get(getattr(finding, "severity", ""), 0) >= minimum for finding in findings) + + def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: parser = argparse.ArgumentParser(description="Run deterministic code review over a unified diff.") parser.add_argument( "--diff-file", default=_default_diff_path(), - help="Path to a unified diff file. Defaults to fixtures/security.diff.", + help="Path to a unified diff file, or '-' to read from stdin. Defaults to fixtures/security.diff.", ) parser.add_argument( "--output-dir", @@ -57,23 +93,34 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: default="", help="Optional SQLite database path for persisting review tasks, findings, and reports.", ) + parser.add_argument( + "--fail-on-severity", + choices=("never", "low", "medium", "high"), + default="never", + help="Exit with status 1 when findings meet or exceed this severity. Defaults to never.", + ) + parser.add_argument( + "--list-rules", + action="store_true", + help="Print deterministic rule metadata and exit without reading a diff.", + ) return parser.parse_args(argv) def main(argv: Sequence[str] | None = None) -> int: started = time.perf_counter() args = parse_args(argv) - diff_file = _resolve_path(args.diff_file) + if args.list_rules: + _print_rules() + return 0 + output_dir = _resolve_path(args.output_dir) db_path = _resolve_path(args.db_path) if args.db_path else None - - if not diff_file.exists(): - raise FileNotFoundError(f"diff file not found: {diff_file}") - - diff_text = diff_file.read_text(encoding="utf-8") + diff_display, report_diff_file, diff_text = _read_diff(args.diff_file) parsed_diff = parse_unified_diff(diff_text) filter_decision = evaluate_filter_decision(diff_text, parsed_diff) findings = run_static_rules(parsed_diff.changed_lines) + failure_gate_triggered = _failure_gate_triggered(findings, args.fail_on_severity) sandbox_run = DryRunSandboxRunner().run( files=parsed_diff.files, findings=findings, @@ -88,7 +135,7 @@ def main(argv: Sequence[str] | None = None) -> int: duration_ms=duration_ms, ) report = build_report( - diff_file=args.diff_file, + diff_file=report_diff_file, files=parsed_diff.files, findings=findings, dry_run=args.dry_run, @@ -111,17 +158,23 @@ def main(argv: Sequence[str] | None = None) -> int: low_count = report["summary"]["severity_counts"].get("low", 0) print("Skills code review dry-run complete") - print(f"Diff file: {diff_file}") + print(f"Diff file: {diff_display}") print(f"Changed files: {len(parsed_diff.files)}") print(f"Findings: high={high_count} medium={medium_count} low={low_count}") print(f"Filter decision: {filter_decision.decision} ({filter_decision.reason})") print(f"Sandbox status: {sandbox_run.status}") + if args.fail_on_severity == "never": + print("Failure gate: disabled (--fail-on-severity never)") + elif failure_gate_triggered: + print(f"Failure gate: triggered (--fail-on-severity {args.fail_on_severity})") + else: + print(f"Failure gate: passed (--fail-on-severity {args.fail_on_severity})") print(f"JSON report: {json_path}") print(f"Markdown report: {md_path}") if db_path is not None: print(f"Database: {db_path}") print(f"Task ID: {task_id}") - return 0 + return 1 if failure_gate_triggered else 0 if __name__ == "__main__": diff --git a/examples/skills_code_review_agent/tests/test_static_review.py b/examples/skills_code_review_agent/tests/test_static_review.py index 411e6c11..963b4be5 100644 --- a/examples/skills_code_review_agent/tests/test_static_review.py +++ b/examples/skills_code_review_agent/tests/test_static_review.py @@ -2,6 +2,8 @@ from __future__ import annotations +import io +import json import sqlite3 from pathlib import Path @@ -15,6 +17,7 @@ from agent.sandbox import DryRunSandboxRunner from agent.storage import persist_review from agent.telemetry import build_telemetry_summary +from run_agent import main as run_agent_main EXAMPLE_ROOT = Path(__file__).resolve().parents[1] @@ -178,3 +181,72 @@ def test_sqlite_persists_sandbox_and_filter_rows(tmp_path: Path) -> None: assert filter_count == 1 assert sandbox_status == "completed" assert filter_decision == "allow" + + +def test_fail_on_severity_high_returns_nonzero(tmp_path: Path, capsys) -> None: + exit_code = run_agent_main( + [ + "--diff-file", + str(FIXTURES / "security.diff"), + "--output-dir", + str(tmp_path), + "--dry-run", + "--fail-on-severity", + "high", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 1 + assert "Failure gate: triggered (--fail-on-severity high)" in captured.out + + +def test_fail_on_severity_never_returns_zero(tmp_path: Path, capsys) -> None: + exit_code = run_agent_main( + [ + "--diff-file", + str(FIXTURES / "security.diff"), + "--output-dir", + str(tmp_path), + "--dry-run", + "--fail-on-severity", + "never", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 0 + assert "Failure gate: disabled (--fail-on-severity never)" in captured.out + + +def test_list_rules_outputs_rule_metadata(capsys) -> None: + exit_code = run_agent_main(["--list-rules", "--diff-file", "does-not-exist.diff"]) + captured = capsys.readouterr() + + assert exit_code == 0 + assert "Deterministic rules" in captured.out + assert "static-rule:hardcoded-secret" in captured.out + assert "category: secret" in captured.out + assert "limitations:" in captured.out + + +def test_diff_file_stdin_reads_unified_diff(tmp_path: Path, monkeypatch, capsys) -> None: + diff_text = (FIXTURES / "missing_timeout.diff").read_text(encoding="utf-8") + monkeypatch.setattr("sys.stdin", io.StringIO(diff_text)) + + exit_code = run_agent_main( + [ + "--diff-file", + "-", + "--output-dir", + str(tmp_path), + "--dry-run", + ] + ) + captured = capsys.readouterr() + report = json.loads((tmp_path / "review_report.json").read_text(encoding="utf-8")) + + assert exit_code == 0 + assert "Diff file: " in captured.out + assert report["summary"]["diff_file"] == "" + assert report["summary"]["category_counts"]["network-timeout"] == 1