diff --git a/examples/skills_code_review_agent/DESIGN.md b/examples/skills_code_review_agent/DESIGN.md new file mode 100644 index 000000000..457eb7643 --- /dev/null +++ b/examples/skills_code_review_agent/DESIGN.md @@ -0,0 +1,9 @@ +# Skills Code Review Agent 方案设计 + +本示例以 `code-review` Skill 描述审查流程和规则，由 Agent 通过受控工具加载 Skill，并调用固定扫描脚本处理统一格式的 diff。输入层支持 diff 文件、文件列表、Git 工作区及内置 fixture，保留文件、hunk 和候选行号，二进制及非法路径转为明确告警或错误。 + +生产默认使用禁网 Container workspace；local runtime 仅在显式环境变量开启时作为开发后备。执行计划固定命令、工作目录、输入输出路径、环境变量名、超时和输出上限，并计算摘要。Filter 在执行前校验计划完整性、路径、命令、环境、网络与预算，先持久化决定；`DENY` 和 `NEEDS_HUMAN_REVIEW` 不进入沙箱。 + +SQLite 默认保存 review task、输入摘要、Filter 决定、sandbox run、finding 和最终报告，存储接口复用 SQLAlchemy/`SqlStorage`，可通过 SQL URL 切换受支持后端。Finding 使用固定结构，按任务、文件、行号和类别去重；低置信结果进入 warning 与人工复核，不混入高置信发现。 + +统一脱敏器覆盖沙箱输出、异常、数据库及 JSON/Markdown 报告，原始 diff 不入库。报告记录总耗时、沙箱耗时、工具调用、拦截次数、finding 数量、严重级别和异常类型分布。超时、输出超限或沙箱失败均形成可查询记录，不使评审流程崩溃；fake model 无需 API Key，仍执行解析、Filter、沙箱和落库主链。 diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md new file mode 100644 index 000000000..a4b819971 --- /dev/null +++ b/examples/skills_code_review_agent/README.md @@ -0,0 +1,166 @@ +# Skills Code Review Agent + +This example implements Issue #92 as a standalone, policy-gated code review +workflow. It accepts a unified diff, a file list, a Git workspace, or one of +eight fixtures and writes queryable JSON/Markdown reports plus SQLite audit +records. It does not change framework production APIs. + +## What it includes + +- a `code-review` Skill with deterministic rules and a sandbox script; +- unified diff, file-list, fixture, and Git input adapters; +- structured findings with deduplication and confidence routing; +- a Filter for command, path, environment, network, and budget policy; +- Container execution by default and an explicit local development fallback; +- SQLite-backed task, Filter, sandbox, finding, and report records; +- fake-model and dry-run modes that require no model API key. + +## Quick start + +Docker must be available for the default Container runtime: + +```powershell +uv run python examples/skills_code_review_agent/run_agent.py run ` + --fixture security ` + --runtime container ` + --fake-model +``` + +The command prints the task ID and the two report paths: + +```text +reports//review_report.json +reports//review_report.md +``` + +Use `--dry-run` to parse, filter, audit, and generate a report without invoking +the sandbox: + +```powershell +uv run python examples/skills_code_review_agent/run_agent.py run ` + --fixture clean ` + --dry-run +``` + +## Input modes + +Exactly one input option is required: + +```powershell +# Unified diff +uv run python examples/skills_code_review_agent/run_agent.py run ` + --diff-file change.diff --fake-model + +# UTF-8 file list; paths are relative to the list file's directory +uv run python examples/skills_code_review_agent/run_agent.py run ` + --file-list changed-files.txt --fake-model + +# Staged Git changes +uv run python examples/skills_code_review_agent/run_agent.py run ` + --repo-path . --staged --fake-model + +# Worktree-only Git changes +uv run python examples/skills_code_review_agent/run_agent.py run ` + --repo-path . --worktree --fake-model + +# Revision range +uv run python examples/skills_code_review_agent/run_agent.py run ` + --repo-path . --base main --head HEAD --fake-model +``` + +With `--repo-path` and no range option, the input is `git diff HEAD`. Untracked +files are not included. + +## Runtime security + +Container is the production default and starts with Docker +`network_mode=none`. Only the fixed Skill files and normalized review input are +staged. The Filter audits the immutable execution plan before the sandbox is +called; denied or human-review decisions do not execute. + +Local execution is an unsafe development fallback and is rejected unless it is +explicitly enabled: + +```powershell +$env:TRPC_CODE_REVIEW_ALLOW_UNSAFE_LOCAL = "1" +uv run python examples/skills_code_review_agent/run_agent.py run ` + --fixture security ` + --runtime local ` + --fake-model +``` + +Do not enable the local runtime in production. + +## Real model + +The real-model path uses the same Skill, Filter, sandbox, storage, and report +contracts. Configure an OpenAI-compatible endpoint: + +```powershell +$env:OPENAI_API_KEY = "" +$env:OPENAI_BASE_URL = "https://api.example.invalid/v1" +$env:MODEL_NAME = "model-name" + +uv run python examples/skills_code_review_agent/run_agent.py run ` + --diff-file change.diff ` + --runtime container +``` + +Model credentials are not included in the sandbox environment. + +## Database and query + +The default database is: + +```text +sqlite:///skills_code_review.db +``` + +Override it with `--db-url` or `TRPC_CODE_REVIEW_DB_URL`. Initialize tables +without running a review: + +```powershell +uv run python examples/skills_code_review_agent/scripts/init_db.py ` + --db-url sqlite:///skills_code_review.db +``` + +Query a task and its persisted report: + +```powershell +uv run python examples/skills_code_review_agent/run_agent.py show ` + --task-id +``` + +The report contains findings, warnings requiring human review, Filter +decisions, sandbox summaries, exceptions, metrics, and the final conclusion. +Sensitive text is redacted before report or database persistence. + +## Tests + +Run the lightweight suite: + +```powershell +uv run pytest tests/examples/skills_code_review_agent -q +``` + +Run the coverage gate: + +```powershell +uv run pytest tests/examples/skills_code_review_agent ` + --cov=examples.skills_code_review_agent ` + --cov-report=term-missing ` + --cov-fail-under=90 +``` + +Run the optional real Container integration: + +```powershell +$env:TRPC_CODE_REVIEW_CONTAINER_TEST = "1" +uv run pytest ` + tests/examples/skills_code_review_agent/test_acceptance.py ` + -k container -q +``` + +Without that environment variable, only the Container integration test is +skipped. SQLite concurrency tests use simultaneous writes and unique-key merge +handling; Python has no Go-style race detector. 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 000000000..4606be6e1 --- /dev/null +++ b/examples/skills_code_review_agent/agent/__init__.py @@ -0,0 +1,19 @@ +"""Core components for the skills code review example.""" + +from .input_parser import load_diff_file +from .input_parser import load_file_list +from .input_parser import load_repo_diff +from .input_parser import parse_diff_text +from .input_parser import GitDiffOptions +from .models import Finding +from .models import ReviewInput + +__all__ = [ + "Finding", + "GitDiffOptions", + "ReviewInput", + "load_diff_file", + "load_file_list", + "load_repo_diff", + "parse_diff_text", +] diff --git a/examples/skills_code_review_agent/agent/constants.py b/examples/skills_code_review_agent/agent/constants.py new file mode 100644 index 000000000..61af04426 --- /dev/null +++ b/examples/skills_code_review_agent/agent/constants.py @@ -0,0 +1,65 @@ +"""Named limits and defaults used by the code review example.""" + +from pathlib import Path + +APP_NAME = "skills_code_review_agent" +DEFAULT_DB_URL = "sqlite:///skills_code_review.db" +DEFAULT_OUTPUT_DIR = Path("reports") +DEFAULT_CONTEXT_LINES = 3 + +MAX_DIFF_BYTES = 2 * 1024 * 1024 +MAX_FILE_BYTES = 512 * 1024 +MAX_FILES = 200 +MAX_HUNKS_PER_FILE = 500 +MAX_CHANGED_LINES = 20_000 +MAX_PATH_LENGTH = 512 +MAX_OUTPUT_BYTES = 64 * 1024 +MAX_JSONL_LINES = 2_000 +MAX_JSONL_LINE_BYTES = 16 * 1024 +MAX_JSON_DEPTH = 12 +MAX_TEXT_FIELD_LENGTH = 8_192 + +GIT_TIMEOUT_SECONDS = 15.0 +FILTER_TIMEOUT_SECONDS = 5.0 +SANDBOX_TIMEOUT_SECONDS = 30.0 +CLEANUP_TIMEOUT_SECONDS = 5.0 +MODEL_TIMEOUT_SECONDS = 45.0 +REPORT_TIMEOUT_SECONDS = 5.0 +TOTAL_TIMEOUT_SECONDS = 90.0 + +MIN_ACTIONABLE_CONFIDENCE = 0.70 +MAX_RETRY_ATTEMPTS = 2 +SQLITE_BUSY_TIMEOUT_MILLISECONDS = 5_000 +DB_ID_LENGTH = 64 +DB_STATUS_LENGTH = 32 +DB_CATEGORY_LENGTH = 64 +DB_PATH_LENGTH = 512 +DB_TITLE_LENGTH = 512 +DB_SOURCE_LENGTH = 256 + +ALLOWED_ENV_NAMES = frozenset({ + "LANG", + "LC_ALL", + "PYTHONIOENCODING", + "PYTHONUTF8", +}) +ALLOWED_INTERPRETERS = frozenset({"python", "python3"}) +BLOCKED_PATH_PARTS = frozenset({ + ".git", + ".ssh", + ".aws", + ".config", + "etc", + "proc", + "sys", +}) + +REDACTION_MARKER = "[REDACTED]" +LOCAL_RUNTIME_OPT_IN_ENV = "TRPC_CODE_REVIEW_ALLOW_UNSAFE_LOCAL" +CONTAINER_TEST_ENV = "TRPC_CODE_REVIEW_CONTAINER_TEST" +DB_URL_ENV = "TRPC_CODE_REVIEW_DB_URL" +ASYNC_SQL_DRIVER_MARKERS = ( + "+aiosqlite", + "+asyncpg", + "+aiomysql", +) diff --git a/examples/skills_code_review_agent/agent/input_parser.py b/examples/skills_code_review_agent/agent/input_parser.py new file mode 100644 index 000000000..01af6ef8f --- /dev/null +++ b/examples/skills_code_review_agent/agent/input_parser.py @@ -0,0 +1,350 @@ +"""Parse diff, file-list, and Git workspace inputs into one contract.""" + +from __future__ import annotations + +import hashlib +import json +import os +import re +import subprocess +from dataclasses import dataclass +from pathlib import Path +from pathlib import PurePosixPath +from pathlib import PureWindowsPath + +from .constants import GIT_TIMEOUT_SECONDS +from .constants import MAX_CHANGED_LINES +from .constants import MAX_DIFF_BYTES +from .constants import MAX_FILE_BYTES +from .constants import MAX_FILES +from .constants import MAX_HUNKS_PER_FILE +from .constants import MAX_PATH_LENGTH +from .models import ChangedLine +from .models import DiffHunk +from .models import InputKind +from .models import LineKind +from .models import ReviewFile +from .models import ReviewInput + +_HUNK_PATTERN = re.compile(r"^@@ -(?P\d+)(?:,(?P\d+))? " + r"\+(?P\d+)(?:,(?P\d+))? @@", ) +_DIFF_HEADER_PATTERN = re.compile(r"^diff --git a/(.+) b/(.+)$") +_NO_NEWLINE_MARKER = "\\ No newline at end of file" +_REVISION_PATTERN = re.compile(r"^[A-Za-z0-9._/@{}~^:+]+$") + + +class InputValidationError(ValueError): + """Raised when review input is unsafe or malformed.""" + + +@dataclass(frozen=True) +class GitDiffOptions: + """Safe Git diff selection options.""" + + staged: bool = False + worktree: bool = False + base: str | None = None + head: str | None = None + + +def _digest(data: bytes) -> str: + return hashlib.sha256(data).hexdigest() + + +def _check_size(data: bytes, limit: int, label: str) -> None: + if len(data) > limit: + raise InputValidationError(f"{label} exceeds {limit} bytes") + if b"\x00" in data: + raise InputValidationError(f"{label} contains NUL bytes") + + +def _normalize_path(raw_path: str) -> str | None: + path = raw_path.strip().split("\t", maxsplit=1)[0] + if path == "/dev/null": + return None + for prefix in ("a/", "b/"): + if path.startswith(prefix): + path = path[len(prefix):] + break + path = path.replace("\\", "/") + posix_path = PurePosixPath(path) + windows_path = PureWindowsPath(path) + if (not path or "\x00" in path or posix_path.is_absolute() or windows_path.is_absolute() or windows_path.drive + or ".." in posix_path.parts): + raise InputValidationError(f"unsafe path: {raw_path}") + if len(path) > MAX_PATH_LENGTH: + raise InputValidationError("path exceeds configured limit") + return path + + +def _parse_hunk_header(header: str) -> tuple[int, int, int, int]: + match = _HUNK_PATTERN.match(header) + if not match: + raise InputValidationError(f"invalid hunk header: {header}") + old_count = int(match.group("old_count") or 1) + new_count = int(match.group("new_count") or 1) + return int(match.group("old")), old_count, int(match.group("new")), new_count + + +def _parse_hunk_lines(lines: list[str], old_start: int, new_start: int) -> list[ChangedLine]: + changed: list[ChangedLine] = [] + old_line = old_start + new_line = new_start + for raw_line in lines: + if raw_line == _NO_NEWLINE_MARKER: + continue + prefix, content = raw_line[:1], raw_line[1:] + if prefix == "+": + changed.append(ChangedLine(kind=LineKind.ADDED, content=content, new_line=new_line)) + new_line += 1 + elif prefix == "-": + changed.append(ChangedLine(kind=LineKind.DELETED, content=content, old_line=old_line)) + old_line += 1 + elif prefix == " ": + changed.append(ChangedLine( + kind=LineKind.CONTEXT, + content=content, + old_line=old_line, + new_line=new_line, + ), ) + old_line += 1 + new_line += 1 + else: + raise InputValidationError(f"invalid hunk line prefix: {prefix!r}") + return changed + + +def _collect_hunk(raw_lines: list[str], start: int) -> tuple[DiffHunk, int]: + header = raw_lines[start] + old_start, old_count, new_start, new_count = _parse_hunk_header(header) + cursor = start + 1 + body: list[str] = [] + seen_old = 0 + seen_new = 0 + while cursor < len(raw_lines): + if seen_old == old_count and seen_new == new_count: + break + line = raw_lines[cursor] + if line.startswith("@@ ") or line.startswith("diff --git "): + break + body.append(line) + if line != _NO_NEWLINE_MARKER: + seen_old += int(not line.startswith("+")) + seen_new += int(not line.startswith("-")) + cursor += 1 + changed = _parse_hunk_lines(body, old_start, new_start) + actual_old = sum(line.kind != LineKind.ADDED for line in changed) + actual_new = sum(line.kind != LineKind.DELETED for line in changed) + if (actual_old, actual_new) != (old_count, new_count): + raise InputValidationError( + f"hunk count mismatch: declared {old_count}/{new_count}, " + f"parsed {actual_old}/{actual_new}", ) + return DiffHunk( + header=header, + old_start=old_start, + old_count=old_count, + new_start=new_start, + new_count=new_count, + lines=changed, + ), cursor + + +def _parse_files(raw_lines: list[str]) -> list[ReviewFile]: + files: list[ReviewFile] = [] + current: ReviewFile | None = None + cursor = 0 + while cursor < len(raw_lines): + line = raw_lines[cursor] + header = _DIFF_HEADER_PATTERN.match(line) + if header: + current = ReviewFile( + old_path=_normalize_path(header.group(1)), + new_path=_normalize_path(header.group(2)), + ) + files.append(current) + elif line.startswith("Binary files ") or line.startswith("GIT binary patch"): + if current: + current.is_binary = True + elif line.startswith("--- "): + if current is None or current.hunks: + current = ReviewFile(old_path=_normalize_path(line[4:])) + files.append(current) + else: + current.old_path = _normalize_path(line[4:]) + elif line.startswith("+++ ") and current: + current.new_path = _normalize_path(line[4:]) + elif line.startswith("@@ ") and current: + hunk, cursor = _collect_hunk(raw_lines, cursor) + current.hunks.append(hunk) + continue + cursor += 1 + return files + + +def _validate_parsed_files(files: list[ReviewFile]) -> None: + if len(files) > MAX_FILES: + raise InputValidationError(f"input exceeds {MAX_FILES} files") + changed_lines = 0 + for review_file in files: + if len(review_file.hunks) > MAX_HUNKS_PER_FILE: + raise InputValidationError("file exceeds configured hunk limit") + changed_lines += sum(len(hunk.lines) for hunk in review_file.hunks) + if changed_lines > MAX_CHANGED_LINES: + raise InputValidationError("input exceeds configured line limit") + + +def parse_diff_text(text: str, source: str = "inline") -> ReviewInput: + """Parse a unified diff string.""" + raw = text.encode("utf-8") + _check_size(raw, MAX_DIFF_BYTES, "diff") + files = _parse_files(text.splitlines()) + if not files: + raise InputValidationError("input contains no Git diff headers") + _validate_parsed_files(files) + warnings = [f"binary file skipped: {item.new_path or item.old_path}" for item in files if item.is_binary] + return ReviewInput( + kind=InputKind.DIFF, + source=source, + digest=_digest(raw), + files=files, + warnings=warnings, + ) + + +def load_diff_file(path: Path) -> ReviewInput: + """Load and parse one UTF-8 unified diff file.""" + data = path.read_bytes() + _check_size(data, MAX_DIFF_BYTES, "diff") + try: + text = data.decode("utf-8") + except UnicodeDecodeError as exc: + raise InputValidationError("diff must be UTF-8") from exc + return parse_diff_text(text, source=str(path)) + + +def _resolve_listed_file(root: Path, relative_path: str) -> Path: + normalized = _normalize_path(relative_path) + if normalized is None: + raise InputValidationError("file list cannot contain /dev/null") + root = root.resolve() + lexical = root / normalized + candidate = lexical.resolve() + if candidate != root and root not in candidate.parents: + raise InputValidationError(f"file escapes repository: {relative_path}") + lexical_path = os.path.normcase(os.path.abspath(lexical)) + resolved_path = os.path.normcase(str(candidate)) + if lexical_path != resolved_path or not candidate.is_file(): + raise InputValidationError(f"file is missing or symbolic: {relative_path}") + return candidate + + +def _file_as_added_hunk(path: Path, root: Path) -> ReviewFile: + data = path.read_bytes() + _check_size(data, MAX_FILE_BYTES, str(path)) + try: + lines = data.decode("utf-8").splitlines() + except UnicodeDecodeError as exc: + raise InputValidationError(f"file must be UTF-8: {path}") from exc + changed = [ + ChangedLine(kind=LineKind.ADDED, content=line, new_line=index) for index, line in enumerate(lines, start=1) + ] + relative = path.relative_to(root).as_posix() + hunk = DiffHunk( + header=f"@@ -0,0 +1,{len(lines)} @@", + old_start=0, + old_count=0, + new_start=1, + new_count=len(lines), + lines=changed, + ) + return ReviewFile(new_path=relative, hunks=[hunk]) + + +def load_file_list(path: Path, repo_path: Path | None = None) -> ReviewInput: + """Load repository-relative UTF-8 files named in a text file.""" + data = path.read_bytes() + _check_size(data, MAX_FILE_BYTES, "file list") + root = (repo_path or path.parent).resolve() + try: + names = [line.strip() for line in data.decode("utf-8").splitlines() if line.strip()] + except UnicodeDecodeError as exc: + raise InputValidationError("file list must be UTF-8") from exc + if not names: + raise InputValidationError("file list must not be empty") + if len(names) > MAX_FILES: + raise InputValidationError(f"file list exceeds {MAX_FILES} entries") + files = [_file_as_added_hunk(_resolve_listed_file(root, name), root) for name in names] + _validate_parsed_files(files) + canonical = json.dumps( + [item.model_dump(mode="json") for item in files], + ensure_ascii=False, + sort_keys=True, + separators=(",", ":"), + ).encode("utf-8") + return ReviewInput( + kind=InputKind.FILE_LIST, + source=str(path), + digest=_digest(canonical), + files=files, + ) + + +def _run_git_diff(repo_path: Path, arguments: list[str]) -> str: + command = [ + "git", + "-C", + str(repo_path), + "-c", + "core.quotePath=false", + "diff", + "--no-ext-diff", + "--no-color", + *arguments, + ] + try: + result = subprocess.run( + command, + check=False, + capture_output=True, + timeout=GIT_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired as exc: + raise InputValidationError("git diff timed out") from exc + _check_size(result.stdout, MAX_DIFF_BYTES, "git diff") + if result.returncode: + error = result.stderr.decode("utf-8", errors="replace").strip() + raise InputValidationError(f"git diff failed: {error}") + return result.stdout.decode("utf-8") + + +def _validate_revision(revision: str) -> str: + if revision.startswith("-") or not _REVISION_PATTERN.fullmatch(revision): + raise InputValidationError(f"unsafe Git revision: {revision!r}") + return revision + + +def load_repo_diff( + repo_path: Path, + options: GitDiffOptions | None = None, +) -> ReviewInput: + """Read a worktree, staged, or revision-range diff from a Git repository.""" + root = repo_path.resolve() + options = options or GitDiffOptions() + if not (root / ".git").exists(): + raise InputValidationError(f"not a Git repository: {root}") + if options.base or options.head: + if not options.base or not options.head: + raise InputValidationError("base and head must be provided together") + arguments = [ + _validate_revision(options.base), + _validate_revision(options.head), + "--", + ] + elif options.staged: + arguments = ["--cached"] + elif options.worktree: + arguments = [] + else: + arguments = ["HEAD"] + parsed = parse_diff_text(_run_git_diff(root, arguments), source=str(root)) + return parsed.model_copy(update={"kind": InputKind.REPOSITORY}) diff --git a/examples/skills_code_review_agent/agent/models.py b/examples/skills_code_review_agent/agent/models.py new file mode 100644 index 000000000..a9cfc43e6 --- /dev/null +++ b/examples/skills_code_review_agent/agent/models.py @@ -0,0 +1,225 @@ +"""Typed contracts for review input, execution, findings, and reports.""" + +from __future__ import annotations + +from datetime import datetime +from enum import Enum +from pathlib import PurePosixPath +from pathlib import PureWindowsPath +from typing import Any + +from pydantic import BaseModel +from pydantic import ConfigDict +from pydantic import Field +from pydantic import field_validator + + +class StrictModel(BaseModel): + """Base model that rejects unknown fields.""" + + model_config = ConfigDict(extra="forbid") + + +class InputKind(str, Enum): + """Supported review input kinds.""" + + DIFF = "diff" + FILE_LIST = "file_list" + REPOSITORY = "repository" + FIXTURE = "fixture" + + +class LineKind(str, Enum): + """Unified diff line kinds.""" + + CONTEXT = "context" + ADDED = "added" + DELETED = "deleted" + + +class Severity(str, Enum): + """Finding severity levels.""" + + CRITICAL = "critical" + HIGH = "high" + MEDIUM = "medium" + LOW = "low" + WARNING = "warning" + + +class Category(str, Enum): + """Rule categories required by Issue #92.""" + + SECURITY = "security" + ASYNC_ERROR = "async_error" + RESOURCE_LEAK = "resource_leak" + MISSING_TEST = "missing_test" + SECRET_LEAK = "secret_leak" + DB_LIFECYCLE = "db_lifecycle" + + +class DecisionAction(str, Enum): + """Policy decision actions.""" + + ALLOW = "allow" + DENY = "deny" + NEEDS_HUMAN_REVIEW = "needs_human_review" + + +class TaskStatus(str, Enum): + """Persisted review task states.""" + + CREATED = "created" + FILTERED = "filtered" + RUNNING = "running" + COMPLETE = "complete" + PARTIAL = "partial" + FAILED = "failed" + DENIED = "denied" + + +class SandboxStatus(str, Enum): + """Sandbox execution states.""" + + SKIPPED = "skipped" + SUCCEEDED = "succeeded" + FAILED = "failed" + TIMED_OUT = "timed_out" + BLOCKED = "blocked" + + +class ChangedLine(StrictModel): + """One line in a unified diff hunk.""" + + kind: LineKind + content: str + old_line: int | None = Field(default=None, ge=1) + new_line: int | None = Field(default=None, ge=1) + + +class DiffHunk(StrictModel): + """A parsed unified diff hunk.""" + + header: str + old_start: int = Field(ge=0) + old_count: int = Field(ge=0) + new_start: int = Field(ge=0) + new_count: int = Field(ge=0) + lines: list[ChangedLine] + + +class ReviewFile(StrictModel): + """A file and its changed hunks.""" + + old_path: str | None = None + new_path: str | None = None + is_binary: bool = False + hunks: list[DiffHunk] = Field(default_factory=list) + + +class ReviewInput(StrictModel): + """Canonical input passed to the review Skill.""" + + kind: InputKind + source: str + digest: str + files: list[ReviewFile] + warnings: list[str] = Field(default_factory=list) + + +class Finding(StrictModel): + """The exact nine-field finding contract required by Issue #92.""" + + severity: Severity + category: Category + file: str + line: int | None + title: str + evidence: str + recommendation: str + confidence: float = Field(ge=0.0, le=1.0) + source: str + + @field_validator("file") + @classmethod + def validate_relative_file(cls, value: str) -> str: + """Reject absolute and parent-traversing finding paths.""" + normalized = value.replace("\\", "/") + posix_path = PurePosixPath(normalized) + windows_path = PureWindowsPath(value) + if (not normalized or "\x00" in normalized or posix_path.is_absolute() or windows_path.is_absolute() + or windows_path.drive or ".." in posix_path.parts): + raise ValueError("finding file must be repository-relative") + return normalized + + +class ExecutionPlan(StrictModel): + """Immutable sandbox request approved by policy.""" + + model_config = ConfigDict(extra="forbid", frozen=True) + + argv: tuple[str, ...] + cwd: str + input_path: str + output_path: str + environment: tuple[tuple[str, str], ...] = () + runtime: str + network_allowed: bool = False + timeout_seconds: float = Field(gt=0) + output_limit_bytes: int = Field(gt=0) + input_digest: str + skill_digest: str + digest: str + + +class FilterDecision(StrictModel): + """One auditable policy decision.""" + + action: DecisionAction + rule_id: str + reason: str + plan_digest: str + created_at: datetime + + +class SandboxRun(StrictModel): + """Normalized result from a sandbox invocation.""" + + status: SandboxStatus + exit_code: int | None = None + timed_out: bool = False + duration_ms: int = Field(ge=0) + stdout: str = "" + stderr: str = "" + error_type: str | None = None + + +class ReviewMetrics(StrictModel): + """Low-cardinality metrics included in a report.""" + + total_duration_ms: int = Field(ge=0) + sandbox_duration_ms: int = Field(ge=0) + tool_calls: int = Field(ge=0) + blocked_count: int = Field(ge=0) + finding_count: int = Field(ge=0) + filter_actions: dict[str, int] = Field(default_factory=dict) + findings_by_severity: dict[str, int] = Field(default_factory=dict) + findings_by_category: dict[str, int] = Field(default_factory=dict) + exceptions_by_type: dict[str, int] = Field(default_factory=dict) + + +class ReviewReport(StrictModel): + """Complete queryable review report.""" + + task_id: str + status: TaskStatus + input_summary: dict[str, Any] + findings: list[Finding] + warnings: list[Finding] + needs_human_review: list[Finding] + filter_decisions: list[FilterDecision] + sandbox_runs: list[SandboxRun] + failures: list[str] + metrics: ReviewMetrics + conclusion: str + created_at: datetime diff --git a/examples/skills_code_review_agent/agent/pipeline.py b/examples/skills_code_review_agent/agent/pipeline.py new file mode 100644 index 000000000..eb49e9636 --- /dev/null +++ b/examples/skills_code_review_agent/agent/pipeline.py @@ -0,0 +1,568 @@ +"""End-to-end Agent, Skill, Filter, sandbox, storage, and report pipeline.""" + +from __future__ import annotations + +import asyncio +import hashlib +import json +import os +import time +import uuid +from dataclasses import dataclass +from dataclasses import field +from datetime import datetime +from datetime import timezone +from pathlib import Path +import stat +from typing import Any +from typing import AsyncGenerator +from typing import Awaitable +from typing import Callable +from typing import List +from typing_extensions import override + +from trpc_agent_sdk.agents import LlmAgent +from trpc_agent_sdk.models import LLMModel +from trpc_agent_sdk.models import LlmRequest +from trpc_agent_sdk.models import LlmResponse +from trpc_agent_sdk.runners import Runner +from trpc_agent_sdk.sessions import InMemorySessionService +from trpc_agent_sdk.skills import SkillToolSet +from trpc_agent_sdk.skills import create_default_skill_repository +from trpc_agent_sdk.skills.tools import CopySkillStager +from trpc_agent_sdk.tools import FunctionTool +from trpc_agent_sdk.types import Content +from trpc_agent_sdk.types import FunctionCall +from trpc_agent_sdk.types import Part + +from .constants import APP_NAME +from .constants import CLEANUP_TIMEOUT_SECONDS +from .constants import MODEL_TIMEOUT_SECONDS +from .constants import TOTAL_TIMEOUT_SECONDS +from .models import DecisionAction +from .models import ReviewInput +from .models import ReviewMetrics +from .models import ReviewReport +from .models import SandboxStatus +from .models import TaskStatus +from .policy import ReviewPolicyFilter +from .policy import SecretRedactor +from .policy import build_execution_plan +from .policy import run_guarded +from .reporting import FindingBuckets +from .reporting import FindingOutputError +from .reporting import parse_findings_jsonl +from .reporting import prepare_findings +from .reporting import write_report_files +from .sandbox import RuntimeHandle +from .sandbox import SandboxExecution +from .sandbox import SandboxExecutor +from .storage import ReviewStore + +MILLISECONDS_PER_SECOND = 1_000 +FAKE_MODEL_NAME = "fake-code-review-model" +SKILL_WORKSPACE_LINK_NAMES = ("out", "work", "inputs") +REVIEW_INSTRUCTION = """ +Review the staged input. Call skill_load for code-review first, then call +review_skill_run exactly once. Never call another tool. Summarize completion +without reproducing source code or credentials. +""".strip() + +ToolAction = Callable[[], Awaitable[SandboxExecution | None]] + + +class FakeReviewModel(LLMModel): + """Deterministic model that performs the same two tool calls.""" + + @classmethod + def supported_models(cls) -> List[str]: + return [FAKE_MODEL_NAME] + + @override + async def _generate_async_impl( + self, + request: LlmRequest, + stream: bool = False, + ctx=None, + ) -> AsyncGenerator[LlmResponse, None]: + del stream, ctx + responses = _function_response_names(request) + if "skill_load" not in responses: + call = FunctionCall( + id="fake-load", + name="skill_load", + args={"skill_name": "code-review"}, + ) + yield LlmResponse(content=Content(role="model", parts=[Part(function_call=call)])) + return + if "review_skill_run" not in responses: + call = FunctionCall( + id="fake-run", + name="review_skill_run", + args={}, + ) + yield LlmResponse(content=Content(role="model", parts=[Part(function_call=call)])) + return + yield LlmResponse(content=Content( + role="model", + parts=[Part(text="Code review Skill completed.")], + ), ) + + def validate_request(self, request: LlmRequest) -> None: + super().validate_request(request) + + +def _function_response_names(request: LlmRequest) -> set[str]: + names = set() + for content in request.contents: + for part in content.parts or []: + if part.function_response: + names.add(part.function_response.name) + return names + + +class _LoadOnlySkillToolSet(SkillToolSet): + """Expose only skill_load; execution goes through the guarded tool.""" + + @override + async def get_tools(self, invocation_context=None): + tools = await super().get_tools(invocation_context) + return [tool for tool in tools if tool.name == "skill_load"] + + +@dataclass +class AgentRunResult: + """Agent events relevant to pipeline metrics and recovery.""" + + execution: SandboxExecution | None + tool_names: list[str] + failures: list[str] + guard_blocked: bool + + +class ReviewAgentExecutor: + """Run an LlmAgent with SkillToolSet and one guarded execution tool.""" + + def __init__(self, dependencies: "AgentDependencies") -> None: + self._model = dependencies.model + self._skill_root = dependencies.skill_root + self._runtime = dependencies.runtime + self._action = dependencies.action + self._execution: SandboxExecution | None = None + self._skill_loaded = False + self._skill_workspace_created = False + self._guard_blocked = False + + async def review_skill_run(self) -> dict[str, Any]: + """Execute the already-planned code-review Skill.""" + if not self._skill_loaded: + self._guard_blocked = True + return {"status": "blocked", "reason": "skill_load is required first"} + if self._execution is None: + self._execution = await self._action() + if self._execution is None: + return {"status": "blocked"} + return { + "status": self._execution.run.status.value, + "finding_output": self._execution.output, + } + + def _create_agent(self) -> LlmAgent: + repository = create_default_skill_repository( + str(self._skill_root), + workspace_runtime=self._runtime.require_runtime(), + ) + skill_tools = _LoadOnlySkillToolSet( + repository=repository, + skill_stager=CopySkillStager(), + create_ws_name_cb=self._skill_workspace_name, + ) + + def after_tool_callback(context, tool, args, response): + del context, args + if tool.name == "skill_load" and _tool_succeeded(response): + self._skill_loaded = True + + return LlmAgent( + name="skills_code_review_agent", + description="Policy-gated code review Agent.", + model=self._model, + instruction=REVIEW_INSTRUCTION, + tools=[skill_tools, FunctionTool(func=self.review_skill_run)], + skill_repository=repository, + after_tool_callback=after_tool_callback, + ) + + def _skill_workspace_name(self, context) -> str: + self._skill_workspace_created = True + return context.session.id + + async def run(self, task_id: str) -> AgentRunResult: + """Run one isolated Agent session with a hard model timeout.""" + agent = self._create_agent() + runner = Runner( + app_name=APP_NAME, + agent=agent, + session_service=InMemorySessionService(), + ) + tool_names: list[str] = [] + failures: list[str] = [] + try: + await asyncio.wait_for( + self._consume(runner, task_id, tool_names, failures), + timeout=MODEL_TIMEOUT_SECONDS, + ) + except Exception as exc: # pylint: disable=broad-except + failures.append(type(exc).__name__) + finally: + await self._cleanup(runner, task_id, failures) + return AgentRunResult( + execution=self._execution, + tool_names=tool_names, + failures=failures, + guard_blocked=self._guard_blocked, + ) + + async def _cleanup( + self, + runner: Runner, + task_id: str, + failures: list[str], + ) -> None: + actions = [("RunnerCloseError", runner.close)] + if self._skill_workspace_created: + actions.append(( + "SkillWorkspaceCleanupError", + lambda: self._cleanup_skill_workspace(task_id), + )) + for label, action in actions: + try: + await asyncio.wait_for(action(), timeout=CLEANUP_TIMEOUT_SECONDS) + except Exception as exc: # pylint: disable=broad-except + failures.append(f"{label}:{type(exc).__name__}") + + async def _cleanup_skill_workspace(self, task_id: str) -> None: + manager = self._runtime.require_runtime().manager() + if self._runtime.kind == "local": + workspace = await manager.create_workspace(task_id) + _make_workspace_removable(Path(workspace.path)) + await manager.cleanup(task_id) + + @staticmethod + async def _consume( + runner: Runner, + task_id: str, + tool_names: list[str], + failures: list[str], + ) -> None: + message = Content(parts=[Part(text="Run the code-review Skill for the prepared input.")]) + async for event in runner.run_async( + user_id="code-review-user", + session_id=task_id, + new_message=message, + ): + if event.error_code or event.error_message: + failures.append(event.error_code or "AgentError") + for part in event.content.parts if event.content else []: + if part.function_call: + tool_names.append(part.function_call.name) + + +@dataclass(frozen=True) +class PipelineDependencies: + """Services and paths needed by the pipeline.""" + + store: ReviewStore + runtime: RuntimeHandle + skill_root: Path + output_dir: Path + model: LLMModel + + +@dataclass(frozen=True) +class AgentDependencies: + """Dependencies used to construct one Agent session.""" + + model: LLMModel + skill_root: Path + runtime: RuntimeHandle + action: ToolAction + + +@dataclass +class _PipelineState: + task_id: str + started: float + decisions: list = field(default_factory=list) + execution: SandboxExecution | None = None + tool_names: list[str] = field(default_factory=list) + failures: list[str] = field(default_factory=list) + attempted: bool = False + + +class ReviewPipeline: + """Coordinate one durable code-review task.""" + + def __init__(self, dependencies: PipelineDependencies) -> None: + self._deps = dependencies + self._redactor = SecretRedactor() + + async def run( + self, + review_input: ReviewInput, + dry_run: bool = False, + ) -> tuple[ReviewReport, Path, Path]: + """Run one task and persist every auditable phase.""" + state = _PipelineState(task_id=uuid.uuid4().hex, started=time.monotonic()) + await self._deps.store.create_task(state.task_id, review_input) + try: + return await asyncio.wait_for( + self._run_task(state, review_input, dry_run), + timeout=TOTAL_TIMEOUT_SECONDS, + ) + except Exception as exc: + failure = self._redactor.redact_text(type(exc).__name__) + await self._deps.store.update_status( + state.task_id, + TaskStatus.FAILED, + failure, + ) + raise + + async def _run_task( + self, + state: _PipelineState, + review_input: ReviewInput, + dry_run: bool, + ) -> tuple[ReviewReport, Path, Path]: + input_bytes = self._serialize_input(review_input) + executor = SandboxExecutor( + self._deps.runtime, + self._redactor, + self._deps.skill_root / "code-review", + ) + plan = build_execution_plan( + self._deps.runtime.kind, + hashlib.sha256(input_bytes).hexdigest(), + executor.skill_digest, + ) + + async def audit(decisions): + state.decisions = decisions + await self._deps.store.save_filter_decisions(state.task_id, decisions) + await self._deps.store.update_status(state.task_id, TaskStatus.FILTERED) + + async def execute_approved(approved): + await self._deps.store.update_status(state.task_id, TaskStatus.RUNNING) + return await executor.execute(approved, input_bytes) + + policy = ReviewPolicyFilter(audit) + + async def tool_action() -> SandboxExecution | None: + if state.attempted: + return state.execution + state.attempted = True + state.execution, _ = await run_guarded( + plan, + policy, + execute_approved, + ) + return state.execution + + if dry_run: + await policy.evaluate_and_audit(plan) + else: + await self._run_agent(state, tool_action) + report = await self._finalize(state, review_input, dry_run) + json_path, markdown_path, markdown = write_report_files( + report, + self._deps.output_dir, + ) + await self._deps.store.complete_task(report, markdown) + return report, json_path, markdown_path + + @staticmethod + def _serialize_input(review_input: ReviewInput) -> bytes: + return json.dumps( + review_input.model_dump(mode="json"), + ensure_ascii=False, + sort_keys=True, + separators=(",", ":"), + ).encode("utf-8") + + async def _run_agent( + self, + state: _PipelineState, + tool_action: ToolAction, + ) -> None: + agent = ReviewAgentExecutor( + AgentDependencies( + model=self._deps.model, + skill_root=self._deps.skill_root, + runtime=self._deps.runtime, + action=tool_action, + ), ) + result = await agent.run(state.task_id) + state.execution = result.execution + state.tool_names = result.tool_names + state.failures.extend(result.failures) + if result.guard_blocked: + state.failures.append("SkillLoadRequired") + if state.execution and "skill_load" not in state.tool_names: + state.failures.append("SkillNotLoadedByAgent") + if state.execution is None and "review_skill_run" not in state.tool_names: + state.failures.append("AgentDidNotRunSkill") + state.execution = await tool_action() + + async def _finalize( + self, + state: _PipelineState, + review_input: ReviewInput, + dry_run: bool, + ) -> ReviewReport: + buckets = FindingBuckets([], [], []) + if state.execution: + await self._deps.store.save_sandbox_run(state.task_id, state.execution.run) + buckets = self._parse_execution(state) + await self._deps.store.save_findings( + state.task_id, + buckets.all_findings, + buckets.human_fingerprints, + ) + status = self._status(state, dry_run) + metrics = self._metrics(state, buckets) + conclusion = self._conclusion(status, buckets) + return ReviewReport( + task_id=state.task_id, + status=status, + input_summary={ + "kind": review_input.kind.value, + "digest": review_input.digest, + "file_count": len(review_input.files), + }, + findings=buckets.actionable, + warnings=buckets.warnings, + needs_human_review=buckets.needs_human_review, + filter_decisions=state.decisions, + sandbox_runs=[state.execution.run] if state.execution else [], + failures=[self._redactor.redact_text(item) for item in state.failures], + metrics=metrics, + conclusion=conclusion, + created_at=datetime_now(), + ) + + def _parse_execution(self, state: _PipelineState) -> FindingBuckets: + execution = state.execution + if execution is None or execution.run.status != SandboxStatus.SUCCEEDED: + return FindingBuckets([], [], []) + try: + findings = parse_findings_jsonl(execution.output, self._redactor) + return prepare_findings(findings, self._redactor) + except FindingOutputError as exc: + state.failures.append(type(exc).__name__) + return FindingBuckets([], [], []) + + @staticmethod + def _status(state: _PipelineState, dry_run: bool) -> TaskStatus: + blocked = any(item.action != DecisionAction.ALLOW for item in state.decisions) + if blocked: + return TaskStatus.DENIED + if dry_run: + return TaskStatus.COMPLETE + if state.execution is None or state.execution.run.status != SandboxStatus.SUCCEEDED: + return TaskStatus.FAILED + if "FindingOutputError" in state.failures: + return TaskStatus.FAILED + if state.failures: + return TaskStatus.PARTIAL + return TaskStatus.COMPLETE + + @staticmethod + def _metrics(state: _PipelineState, buckets: FindingBuckets) -> ReviewMetrics: + execution = state.execution + findings = buckets.all_findings + exceptions = list(state.failures) + if execution and execution.run.error_type: + exceptions.append(execution.run.error_type) + filter_actions: dict[str, int] = {} + for decision in state.decisions: + name = decision.action.value + filter_actions[name] = filter_actions.get(name, 0) + 1 + return ReviewMetrics( + total_duration_ms=int((time.monotonic() - state.started) * MILLISECONDS_PER_SECOND), + sandbox_duration_ms=execution.run.duration_ms if execution else 0, + tool_calls=len(state.tool_names), + blocked_count=sum(count for action, count in filter_actions.items() + if action != DecisionAction.ALLOW.value), + finding_count=len(findings), + filter_actions=filter_actions, + findings_by_severity=_count_values(item.severity.value for item in findings), + findings_by_category=_count_values(item.category.value for item in findings), + exceptions_by_type=_count_values(exceptions), + ) + + @staticmethod + def _conclusion(status: TaskStatus, buckets: FindingBuckets) -> str: + if status == TaskStatus.DENIED: + return "Execution denied by policy; no sandbox command ran." + if status == TaskStatus.FAILED: + return "Review failed; inspect sandbox and exception records." + if not buckets.all_findings: + return "No review findings." + return (f"{len(buckets.actionable)} actionable findings and " + f"{len(buckets.warnings)} warnings.") + + +def _count_values(values) -> dict[str, int]: + counts: dict[str, int] = {} + for value in values: + counts[value] = counts.get(value, 0) + 1 + return counts + + +def _tool_succeeded(response: Any) -> bool: + if response is None: + return False + if isinstance(response, dict): + return not response.get("error") + return True + + +def _make_workspace_removable(root: Path) -> None: + skill_dir = root / "skills" / "code-review" + for path in [root, *root.rglob("*")]: + if _is_workspace_link(path): + continue + try: + path.chmod(path.stat().st_mode | stat.S_IWRITE) + except FileNotFoundError: + continue + for name in SKILL_WORKSPACE_LINK_NAMES: + target = skill_dir / name + if os.path.lexists(target) and _is_workspace_link(target): + _remove_workspace_link(target) + + +def _is_workspace_link(path: Path) -> bool: + if path.is_symlink(): + return True + is_junction = getattr(path, "is_junction", None) + if is_junction and is_junction(): + return True + try: + attributes = path.lstat().st_file_attributes + except (AttributeError, FileNotFoundError): + return False + return bool(attributes & stat.FILE_ATTRIBUTE_REPARSE_POINT) + + +def _remove_workspace_link(path: Path) -> None: + if path.is_symlink(): + path.unlink() + else: + path.rmdir() + + +def datetime_now() -> datetime: + """Return a timezone-aware report timestamp.""" + return datetime.now(timezone.utc) diff --git a/examples/skills_code_review_agent/agent/policy.py b/examples/skills_code_review_agent/agent/policy.py new file mode 100644 index 000000000..79b2b2b1c --- /dev/null +++ b/examples/skills_code_review_agent/agent/policy.py @@ -0,0 +1,273 @@ +"""Secret redaction and fail-closed execution-plan filtering.""" + +from __future__ import annotations + +import hashlib +import json +import os +import re +from collections.abc import Awaitable +from collections.abc import Callable +from datetime import datetime +from datetime import timezone +from pathlib import PurePosixPath +from pathlib import PureWindowsPath +from typing import Any + +from trpc_agent_sdk.abc import FilterResult +from trpc_agent_sdk.abc import FilterType +from trpc_agent_sdk.filter import BaseFilter + +from .constants import ALLOWED_ENV_NAMES +from .constants import ALLOWED_INTERPRETERS +from .constants import BLOCKED_PATH_PARTS +from .constants import LOCAL_RUNTIME_OPT_IN_ENV +from .constants import MAX_JSON_DEPTH +from .constants import MAX_OUTPUT_BYTES +from .constants import REDACTION_MARKER +from .constants import SANDBOX_TIMEOUT_SECONDS +from .models import DecisionAction +from .models import ExecutionPlan +from .models import FilterDecision + +AuditCallback = Callable[[list[FilterDecision]], Awaitable[None]] + +_FIXED_SCRIPT = "scripts/scan_rules.py" +_FIXED_INPUT = "../../work/inputs/review_input.json" +_FIXED_OUTPUT = "../../out/findings.jsonl" +_FIXED_CWD = "skills/code-review" +FIXED_SCAN_ARGV = ( + "python", + _FIXED_SCRIPT, + "--input", + _FIXED_INPUT, + "--output", + _FIXED_OUTPUT, +) +_SUPPORTED_RUNTIMES = frozenset({"container", "local"}) +_PRIVATE_KEY_PATTERN = re.compile( + r"-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----.*?" + r"-----END (?:RSA |EC |OPENSSH )?PRIVATE KEY-----", + re.DOTALL, +) +_ASSIGNMENT_PATTERN = re.compile( + r"(?i)(?P[\"']?(?:api[_-]?key|access[_-]?token|client[_-]?secret|password|密码|密钥|令牌)[\"']?)" + r"(?P\s*[:=]\s*)(?:[\"'][^\"'\r\n]{8,}[\"']|[^\"'\s,;}]{8,})", ) +_SPLIT_ASSIGNMENT_PATTERN = re.compile( + r"(?is)(?P[\"']?(?:api[_-]?key|access[_-]?token|client[_-]?secret|password" + r"|密码|密钥|令牌)[\"']?" + r"\s*[:=]\s*\(?)(?:\s*[\"'][^\"']+[\"']\s*\+?){2,}", ) +_BEARER_PATTERN = re.compile(r"(?i)\bbearer\s+[A-Za-z0-9._~+/=-]{8,}") +_URL_CREDENTIAL_PATTERN = re.compile(r"(https?://[^/\s:@]+):([^@\s/]+)@") +_TOKEN_PATTERN = re.compile( + r"\b(?:AKIA[0-9A-Z]{16}|AIza[0-9A-Za-z_-]{35}|gh[pousr]_[A-Za-z0-9]{30,}|" + r"sk-[A-Za-z0-9_-]{20,}|eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b", ) +_SENSITIVE_NAME_PATTERN = re.compile( + r"(?i)api[_-]?key|access[_-]?token|client[_-]?secret|password|authorization|密码|密钥|令牌", ) + + +class SecretRedactor: + """Redact credentials before text leaves the review process.""" + + def redact_text(self, value: str) -> str: + """Redact supported secret forms from text.""" + redacted = _PRIVATE_KEY_PATTERN.sub(REDACTION_MARKER, value) + redacted = _SPLIT_ASSIGNMENT_PATTERN.sub( + lambda match: f"{match.group('prefix')}{REDACTION_MARKER}", + redacted, + ) + redacted = _ASSIGNMENT_PATTERN.sub( + lambda match: f"{match.group('name')}{match.group('separator')}{REDACTION_MARKER}", + redacted, + ) + redacted = _BEARER_PATTERN.sub(f"Bearer {REDACTION_MARKER}", redacted) + redacted = _URL_CREDENTIAL_PATTERN.sub( + lambda match: f"{match.group(1)}:{REDACTION_MARKER}@", + redacted, + ) + return _TOKEN_PATTERN.sub(REDACTION_MARKER, redacted) + + def redact_value(self, value: Any, depth: int = 0) -> Any: + """Recursively redact JSON-like values with a depth cap.""" + if depth > MAX_JSON_DEPTH: + return REDACTION_MARKER + if isinstance(value, str): + return self.redact_text(value) + if isinstance(value, dict): + return { + key: (REDACTION_MARKER if _SENSITIVE_NAME_PATTERN.fullmatch(str(key)) else self.redact_value( + item, depth + 1)) + for key, item in value.items() + } + if isinstance(value, (list, tuple)): + return [self.redact_value(item, depth + 1) for item in value] + return value + + +def _plan_payload(plan: ExecutionPlan) -> dict[str, Any]: + payload = plan.model_dump(mode="json") + payload.pop("digest", None) + return payload + + +def calculate_plan_digest(plan: ExecutionPlan) -> str: + """Hash every execution-relevant plan field.""" + encoded = json.dumps( + _plan_payload(plan), + ensure_ascii=True, + sort_keys=True, + separators=(",", ":"), + ).encode("utf-8") + return hashlib.sha256(encoded).hexdigest() + + +def build_execution_plan( + runtime: str, + input_digest: str, + skill_digest: str, +) -> ExecutionPlan: + """Build the only execution plan accepted by the example.""" + draft = ExecutionPlan( + argv=FIXED_SCAN_ARGV, + cwd=_FIXED_CWD, + input_path="work/inputs/review_input.json", + output_path="out/findings.jsonl", + environment=( + ("PYTHONIOENCODING", "utf-8"), + ("PYTHONUTF8", "1"), + ), + runtime=runtime, + network_allowed=False, + timeout_seconds=SANDBOX_TIMEOUT_SECONDS, + output_limit_bytes=MAX_OUTPUT_BYTES, + input_digest=input_digest, + skill_digest=skill_digest, + digest="pending", + ) + return draft.model_copy(update={"digest": calculate_plan_digest(draft)}) + + +def _decision( + action: DecisionAction, + rule_id: str, + reason: str, + plan: ExecutionPlan, +) -> FilterDecision: + return FilterDecision( + action=action, + rule_id=rule_id, + reason=reason, + plan_digest=plan.digest, + created_at=datetime.now(timezone.utc), + ) + + +def _is_safe_relative_path(value: str) -> bool: + normalized = value.replace("\\", "/") + posix_path = PurePosixPath(normalized) + windows_path = PureWindowsPath(normalized) + lowered = {part.casefold() for part in posix_path.parts} + return bool(normalized and "\x00" not in normalized and not posix_path.is_absolute() + and not windows_path.is_absolute() and not windows_path.drive and ".." not in posix_path.parts + and not lowered.intersection(BLOCKED_PATH_PARTS)) + + +class ReviewPolicyFilter(BaseFilter): + """Framework-compatible Filter that audits before allowing execution.""" + + def __init__(self, audit: AuditCallback) -> None: + super().__init__() + self.name = "code_review_execution_plan" + self.type = FilterType.TOOL + self._audit = audit + + def evaluate(self, plan: ExecutionPlan) -> list[FilterDecision]: + """Evaluate all fail-closed policy rules.""" + checks = ( + self._check_integrity(plan), + self._check_command(plan), + self._check_paths(plan), + self._check_environment(plan), + self._check_runtime(plan), + self._check_budget(plan), + ) + return list(checks) + + async def evaluate_and_audit(self, plan: ExecutionPlan) -> list[FilterDecision]: + """Persist decisions before the caller may execute.""" + try: + decisions = self.evaluate(plan) + except Exception as exc: # pylint: disable=broad-except + decisions = [ + _decision( + DecisionAction.DENY, + "filter.internal-error", + f"filter failed closed: {type(exc).__name__}", + plan, + ), + ] + await self._audit(decisions) + return decisions + + async def _before(self, ctx: Any, req: Any, rsp: FilterResult) -> None: + if not isinstance(req, ExecutionPlan): + raise TypeError("review policy requires ExecutionPlan") + decisions = await self.evaluate_and_audit(req) + rsp.rsp = decisions + rsp.is_continue = all(item.action == DecisionAction.ALLOW for item in decisions) + + def _check_integrity(self, plan: ExecutionPlan) -> FilterDecision: + valid = plan.digest == calculate_plan_digest(plan) + action = DecisionAction.ALLOW if valid else DecisionAction.DENY + reason = "plan digest verified" if valid else "plan changed after construction" + return _decision(action, "plan.integrity", reason, plan) + + def _check_command(self, plan: ExecutionPlan) -> FilterDecision: + valid = plan.argv == FIXED_SCAN_ARGV and plan.argv[0] in ALLOWED_INTERPRETERS + action = DecisionAction.ALLOW if valid else DecisionAction.DENY + reason = "fixed scanner argv" if valid else "command is not the fixed scanner argv" + return _decision(action, "command.allowlist", reason, plan) + + def _check_paths(self, plan: ExecutionPlan) -> FilterDecision: + paths = (plan.cwd, plan.input_path, plan.output_path) + valid = all(_is_safe_relative_path(path) for path in paths) + action = DecisionAction.ALLOW if valid else DecisionAction.DENY + reason = "workspace-relative paths" if valid else "unsafe execution path" + return _decision(action, "path.workspace", reason, plan) + + def _check_environment(self, plan: ExecutionPlan) -> FilterDecision: + valid = all(name in ALLOWED_ENV_NAMES for name, _ in plan.environment) + action = DecisionAction.ALLOW if valid else DecisionAction.DENY + reason = "environment allowlist" if valid else "environment key is not allowed" + return _decision(action, "environment.allowlist", reason, plan) + + def _check_runtime(self, plan: ExecutionPlan) -> FilterDecision: + supported = plan.runtime in _SUPPORTED_RUNTIMES + local_allowed = os.getenv(LOCAL_RUNTIME_OPT_IN_ENV) == "1" + valid = supported and not plan.network_allowed + if plan.runtime == "local" and not local_allowed: + valid = False + action = DecisionAction.ALLOW if valid else DecisionAction.DENY + if valid and plan.runtime == "local": + reason = "unsafe local runtime explicitly enabled for development" + else: + reason = "runtime isolation policy" if valid else "runtime or network policy rejected" + return _decision(action, "runtime.isolation", reason, plan) + + def _check_budget(self, plan: ExecutionPlan) -> FilterDecision: + valid = (plan.timeout_seconds <= SANDBOX_TIMEOUT_SECONDS and plan.output_limit_bytes <= MAX_OUTPUT_BYTES) + action = DecisionAction.ALLOW if valid else DecisionAction.DENY + reason = "execution budget accepted" if valid else "execution budget exceeded" + return _decision(action, "budget.limit", reason, plan) + + +async def run_guarded( + plan: ExecutionPlan, + policy: ReviewPolicyFilter, + handler: Callable[[ExecutionPlan], Awaitable[Any]], +) -> tuple[Any | None, list[FilterDecision]]: + """Run handler only after every audited decision allows it.""" + decisions = await policy.evaluate_and_audit(plan) + if any(item.action != DecisionAction.ALLOW for item in decisions): + return None, decisions + return await handler(plan), decisions diff --git a/examples/skills_code_review_agent/agent/reporting.py b/examples/skills_code_review_agent/agent/reporting.py new file mode 100644 index 000000000..666533055 --- /dev/null +++ b/examples/skills_code_review_agent/agent/reporting.py @@ -0,0 +1,262 @@ +"""Finding validation, deduplication, and JSON/Markdown reporting.""" + +from __future__ import annotations + +import html +import json +import os +import re +import tempfile +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from .constants import MAX_JSON_DEPTH +from .constants import MAX_JSONL_LINE_BYTES +from .constants import MAX_JSONL_LINES +from .constants import MAX_OUTPUT_BYTES +from .constants import MAX_TEXT_FIELD_LENGTH +from .constants import MIN_ACTIONABLE_CONFIDENCE +from .models import Finding +from .models import ReviewReport +from .models import Severity +from .policy import SecretRedactor +from .storage import finding_fingerprint + +_MARKDOWN_SPECIAL_PATTERN = re.compile(r"([\\`*_[\]{}()#+\-.!|>])") + +FINDING_FIELDS = frozenset({ + "severity", + "category", + "file", + "line", + "title", + "evidence", + "recommendation", + "confidence", + "source", +}) +REPORT_JSON_NAME = "review_report.json" +REPORT_MARKDOWN_NAME = "review_report.md" +MERGE_SEPARATOR = " | " +_SEVERITY_RANK = { + Severity.WARNING: 0, + Severity.LOW: 1, + Severity.MEDIUM: 2, + Severity.HIGH: 3, + Severity.CRITICAL: 4, +} + + +class FindingOutputError(ValueError): + """Raised for malformed or over-budget scanner output.""" + + +@dataclass(frozen=True) +class FindingBuckets: + """Actionable, warning, and human-review finding groups.""" + + actionable: list[Finding] + warnings: list[Finding] + needs_human_review: list[Finding] + + @property + def all_findings(self) -> list[Finding]: + """Return every unique finding once.""" + return [*self.actionable, *self.warnings] + + @property + def human_fingerprints(self) -> set[str]: + """Return storage fingerprints for manual findings.""" + return {finding_fingerprint(item) for item in self.needs_human_review} + + +def _validate_json_depth(value: Any, depth: int = 0) -> None: + if depth > MAX_JSON_DEPTH: + raise FindingOutputError("finding JSON exceeds depth limit") + if isinstance(value, dict): + for item in value.values(): + _validate_json_depth(item, depth + 1) + elif isinstance(value, list): + for item in value: + _validate_json_depth(item, depth + 1) + elif isinstance(value, str) and len(value) > MAX_TEXT_FIELD_LENGTH: + raise FindingOutputError("finding text exceeds length limit") + + +def _parse_finding(line: str, redactor: SecretRedactor) -> Finding: + try: + payload = json.loads(line) + except json.JSONDecodeError as exc: + raise FindingOutputError(f"invalid finding JSON: {exc.msg}") from exc + if not isinstance(payload, dict) or set(payload) != FINDING_FIELDS: + raise FindingOutputError("finding must contain exactly the nine required fields") + _validate_json_depth(payload) + return Finding.model_validate(redactor.redact_value(payload)) + + +def parse_findings_jsonl(text: str, redactor: SecretRedactor) -> list[Finding]: + """Parse bounded JSONL emitted by the sandbox.""" + encoded = text.encode("utf-8") + if len(encoded) > MAX_OUTPUT_BYTES: + raise FindingOutputError("finding output exceeds total byte limit") + lines = [line for line in text.splitlines() if line.strip()] + if len(lines) > MAX_JSONL_LINES: + raise FindingOutputError("finding output exceeds line limit") + findings = [] + for line in lines: + if len(line.encode("utf-8")) > MAX_JSONL_LINE_BYTES: + raise FindingOutputError("finding output line exceeds byte limit") + findings.append(_parse_finding(line, redactor)) + return findings + + +def _merge_text(left: str, right: str) -> str: + values = [] + for value in (left, right): + if value and value not in values: + values.append(value) + return MERGE_SEPARATOR.join(values)[:MAX_TEXT_FIELD_LENGTH] + + +def _merge_findings(left: Finding, right: Finding) -> Finding: + preferred = right + if ( + _SEVERITY_RANK[left.severity], + left.confidence, + ) > ( + _SEVERITY_RANK[right.severity], + right.confidence, + ): + preferred = left + return preferred.model_copy(update={ + "evidence": _merge_text(left.evidence, right.evidence), + "recommendation": _merge_text(left.recommendation, right.recommendation), + "source": _merge_text(left.source, right.source), + "confidence": max(left.confidence, right.confidence), + }, ) + + +def deduplicate_findings(findings: list[Finding]) -> list[Finding]: + """Deduplicate by file, line, and category as required by Issue #92.""" + selected: dict[tuple[str, int | None, str], Finding] = {} + for finding in findings: + key = (finding.file, finding.line, finding.category.value) + previous = selected.get(key) + selected[key] = finding if previous is None else _merge_findings(previous, finding) + return sorted( + selected.values(), + key=lambda item: (item.file, item.line or 0, item.category.value), + ) + + +def prepare_findings( + findings: list[Finding], + redactor: SecretRedactor, +) -> FindingBuckets: + """Redact, deduplicate, and route low-confidence findings.""" + safe = [Finding.model_validate(redactor.redact_value(item.model_dump(mode="json"))) for item in findings] + actionable = [] + warnings = [] + for finding in deduplicate_findings(safe): + if finding.confidence < MIN_ACTIONABLE_CONFIDENCE: + warnings.append(finding.model_copy(update={"severity": Severity.WARNING})) + else: + actionable.append(finding) + return FindingBuckets( + actionable=actionable, + warnings=warnings, + needs_human_review=list(warnings), + ) + + +def render_markdown(report: ReviewReport) -> str: + """Render the fixed report sections required by Issue #92.""" + task_id = _markdown_text(report.task_id) + conclusion = _markdown_text(report.conclusion) + file_count = _markdown_text(report.input_summary.get("file_count", 0)) + lines = [ + f"# Code Review Report: {task_id}", + "", + "## Task summary", + "", + f"- Status: `{report.status.value}`", + f"- Files: {file_count}", + f"- Conclusion: {conclusion}", + "", + "## Findings", + "", + ] + lines.extend(_finding_markdown(report.findings)) + lines.extend(["", "## Warnings / needs human review", ""]) + lines.extend(_finding_markdown(report.warnings)) + lines.extend(["", "## Filter decisions", ""]) + lines.extend(f"- `{item.action.value}` {_markdown_text(item.rule_id)}: {_markdown_text(item.reason)}" + for item in report.filter_decisions) + lines.extend(["", "## Sandbox runs", ""]) + lines.extend(f"- `{item.status.value}` exit={item.exit_code} " + f"timeout={item.timed_out} duration_ms={item.duration_ms}" for item in report.sandbox_runs) + lines.extend(["", "## Exceptions", ""]) + lines.extend(f"- {_markdown_text(failure)}" for failure in report.failures) + lines.extend(["", "## Metrics", "", "```json"]) + lines.append(json.dumps(report.metrics.model_dump(mode="json"), indent=2, sort_keys=True)) + lines.extend(["```", "", "## Final conclusion", "", conclusion, ""]) + return "\n".join(lines) + + +def _markdown_text(value: Any) -> str: + normalized = str(value).replace("\r", " ").replace("\n", " ") + escaped = html.escape(normalized, quote=False) + return _MARKDOWN_SPECIAL_PATTERN.sub(r"\\\1", escaped) + + +def _finding_markdown(findings: list[Finding]) -> list[str]: + if not findings: + return ["- None"] + return [ + f"- **{item.severity.value} / {item.category.value}** " + f"{_markdown_text(item.file)}:{item.line} — {_markdown_text(item.title)} \n" + f" Evidence: {_markdown_text(item.evidence)} \n" + f" Recommendation: {_markdown_text(item.recommendation)} \n" + f" Confidence: {item.confidence:.2f}; Source: {_markdown_text(item.source)}" for item in findings + ] + + +def _atomic_write(path: Path, content: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + descriptor, temporary = tempfile.mkstemp( + prefix=f".{path.name}.", + dir=path.parent, + text=True, + ) + try: + with os.fdopen(descriptor, "w", encoding="utf-8", newline="\n") as output: + output.write(content) + os.replace(temporary, path) + except Exception: + try: + os.unlink(temporary) + except FileNotFoundError: + pass + raise + + +def write_report_files( + report: ReviewReport, + output_dir: Path, +) -> tuple[Path, Path, str]: + """Atomically write JSON and Markdown report files.""" + safe_report = ReviewReport.model_validate(SecretRedactor().redact_value(report.model_dump(mode="json")), ) + markdown = render_markdown(safe_report) + task_dir = output_dir / safe_report.task_id + json_path = task_dir / REPORT_JSON_NAME + markdown_path = task_dir / REPORT_MARKDOWN_NAME + payload = json.dumps( + safe_report.model_dump(mode="json"), + ensure_ascii=False, + indent=2, + sort_keys=True, + ) + _atomic_write(json_path, payload + "\n") + _atomic_write(markdown_path, markdown) + return json_path, markdown_path, markdown 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 000000000..71b10a2f0 --- /dev/null +++ b/examples/skills_code_review_agent/agent/sandbox.py @@ -0,0 +1,376 @@ +"""Sandbox adapter built on repository workspace runtimes.""" + +from __future__ import annotations + +import asyncio +import hashlib +import os +import tempfile +import time +import uuid +from dataclasses import dataclass +from pathlib import Path + +from trpc_agent_sdk.code_executors import BaseWorkspaceRuntime +from trpc_agent_sdk.code_executors import WorkspacePutFileInfo +from trpc_agent_sdk.code_executors import WorkspaceResourceLimits +from trpc_agent_sdk.code_executors import WorkspaceRunProgramSpec +from trpc_agent_sdk.code_executors import create_container_workspace_runtime +from trpc_agent_sdk.code_executors import create_local_workspace_runtime + +from .constants import APP_NAME +from .constants import CLEANUP_TIMEOUT_SECONDS +from .constants import LOCAL_RUNTIME_OPT_IN_ENV +from .constants import MAX_OUTPUT_BYTES +from .models import ExecutionPlan +from .models import SandboxRun +from .models import SandboxStatus +from .policy import SecretRedactor +from .policy import calculate_plan_digest + +CPU_PERCENT_LIMIT = 100 +MEMORY_LIMIT_MB = 256 +PROCESS_LIMIT = 32 +TRUNCATION_SUFFIX = "\n[OUTPUT TRUNCATED]" +MILLISECONDS_PER_SECOND = 1_000 +SKILL_ASSET_PATHS = ( + "SKILL.md", + "references/rules.md", + "scripts/scan_rules.py", +) +SUPPORTED_RUNTIME_KINDS = frozenset({"container", "local"}) + + +@dataclass(frozen=True) +class RuntimeHandle: + """Runtime plus capabilities guaranteed by its factory.""" + + runtime: BaseWorkspaceRuntime | None + kind: str + network_disabled: bool + + def require_runtime(self) -> BaseWorkspaceRuntime: + """Return the initialized runtime or reject execution.""" + if self.runtime is None: + raise RuntimeError("sandbox runtime is not initialized") + return self.runtime + + +@dataclass(frozen=True) +class SandboxExecution: + """Sandbox record and bounded primary output.""" + + run: SandboxRun + output: str + + +@dataclass(frozen=True) +class _RunContext: + started: float + plan: ExecutionPlan + deadline: float + + +@dataclass +class _ExecutionState: + exec_id: str + context: _RunContext + workspace: object | None = None + + +def create_runtime(kind: str, work_root: Path | None = None) -> RuntimeHandle: + """Create a supported runtime with safe defaults.""" + if kind == "container": + runtime = create_container_workspace_runtime( + host_config={"network_mode": "none"}, + auto_inputs=False, + ) + return RuntimeHandle(runtime=runtime, kind=kind, network_disabled=True) + if kind != "local": + raise ValueError(f"unsupported runtime: {kind}") + if os.getenv(LOCAL_RUNTIME_OPT_IN_ENV) != "1": + raise PermissionError("local runtime requires explicit development opt-in") + root = work_root or Path(tempfile.gettempdir()) / APP_NAME + runtime = create_local_workspace_runtime( + work_root=str(root), + read_only_staged_skill=True, + auto_inputs=False, + enable_provider_env=False, + ) + return RuntimeHandle(runtime=runtime, kind=kind, network_disabled=False) + + +def create_plan_runtime(kind: str) -> RuntimeHandle: + """Create a capability-only handle for dry-run policy evaluation.""" + if kind not in SUPPORTED_RUNTIME_KINDS: + raise ValueError(f"unsupported runtime: {kind}") + return RuntimeHandle( + runtime=None, + kind=kind, + network_disabled=kind == "container", + ) + + +def _truncate(value: str, limit: int) -> str: + if limit <= 0: + return "" + encoded = value.encode("utf-8") + if len(encoded) <= limit: + return value + suffix = TRUNCATION_SUFFIX.encode("utf-8") + if limit <= len(suffix): + return encoded[:limit].decode("utf-8", errors="ignore") + content = encoded[:limit - len(suffix)] + return content.decode("utf-8", errors="ignore") + TRUNCATION_SUFFIX + + +def _digest_assets(assets: tuple[WorkspacePutFileInfo, ...]) -> str: + digest = hashlib.sha256() + for asset in assets: + digest.update(asset.path.encode("utf-8")) + digest.update(b"\x00") + digest.update(asset.content) + digest.update(b"\x00") + return digest.hexdigest() + + +def load_skill_assets(skill_dir: Path) -> tuple[WorkspacePutFileInfo, ...]: + """Read the fixed trusted Skill files into an immutable tuple.""" + assets = [] + for relative in SKILL_ASSET_PATHS: + source = skill_dir / relative + assets.append(WorkspacePutFileInfo( + path=f"skills/code-review/{relative}", + content=source.read_bytes(), + ), ) + return tuple(assets) + + +def skill_assets_digest(skill_dir: Path) -> str: + """Calculate the digest embedded in an execution plan.""" + return _digest_assets(load_skill_assets(skill_dir)) + + +class SandboxExecutor: + """Stage fixed inputs, execute one plan, collect output, and clean up.""" + + def __init__( + self, + handle: RuntimeHandle, + redactor: SecretRedactor, + skill_dir: Path, + ) -> None: + self._handle = handle + self._redactor = redactor + self._skill_assets = load_skill_assets(skill_dir) + self._skill_digest = _digest_assets(self._skill_assets) + + @property + def skill_digest(self) -> str: + """Digest of the exact Skill bytes staged for execution.""" + return self._skill_digest + + async def execute( + self, + plan: ExecutionPlan, + input_bytes: bytes, + ) -> SandboxExecution: + """Execute one already-approved immutable plan.""" + self._validate_plan(plan, input_bytes) + started = time.monotonic() + context = _RunContext( + started=started, + plan=plan, + deadline=started + plan.timeout_seconds, + ) + state = _ExecutionState(exec_id=f"review-{uuid.uuid4().hex}", context=context) + execution: SandboxExecution | None = None + try: + state.workspace = await self._within_budget( + self._handle.require_runtime().manager().create_workspace(state.exec_id), + context, + ) + result, output = await self._run_steps(state, input_bytes) + execution = self._normalize_result(result, output, context) + except asyncio.TimeoutError: + execution = self._failure( + SandboxStatus.TIMED_OUT, + asyncio.TimeoutError(), + context, + ) + except Exception as exc: # pylint: disable=broad-except + execution = self._failure(SandboxStatus.FAILED, exc, context) + finally: + cleanup_error = await self._cleanup(state) + if cleanup_error: + return self._record_cleanup_error(execution, cleanup_error, context) + return execution + + def _record_cleanup_error( + self, + execution: SandboxExecution, + error: Exception, + context: _RunContext, + ) -> SandboxExecution: + if execution.run.status == SandboxStatus.SUCCEEDED: + return self._failure(SandboxStatus.FAILED, error, context) + detail = self._redactor.redact_text(str(error)) + limit = min(context.plan.output_limit_bytes, MAX_OUTPUT_BYTES) + stdout, stderr, output = self._bounded_outputs( + execution.run.stdout, + f"{execution.run.stderr}\ncleanup: {detail}".strip(), + execution.output, + limit, + ) + error_type = f"{execution.run.error_type}+{type(error).__name__}" + return SandboxExecution( + run=execution.run.model_copy(update={ + "stdout": stdout, + "stderr": stderr, + "error_type": error_type + }, ), + output=output, + ) + + def _validate_plan(self, plan: ExecutionPlan, input_bytes: bytes) -> None: + if plan.digest != calculate_plan_digest(plan): + raise ValueError("execution plan digest mismatch") + if plan.input_digest != hashlib.sha256(input_bytes).hexdigest(): + raise ValueError("execution input digest mismatch") + if plan.skill_digest != self._skill_digest: + raise ValueError("execution Skill digest mismatch") + if plan.runtime != self._handle.kind: + raise ValueError("execution plan runtime mismatch") + if plan.network_allowed or (plan.runtime == "container" and not self._handle.network_disabled): + raise PermissionError("runtime cannot prove network isolation") + + async def _run_steps(self, state: _ExecutionState, input_bytes: bytes): + input_file = WorkspacePutFileInfo( + path=state.context.plan.input_path, + content=input_bytes, + ) + files = [*self._skill_assets, input_file] + await self._within_budget( + self._handle.require_runtime().fs().put_files(state.workspace, files), + state.context, + ) + result = await self._within_budget( + self._run(state.workspace, state.context.plan), + state.context, + ) + output = await self._within_budget( + self._collect(state.workspace, state.context.plan), + state.context, + ) + return result, output + + async def _within_budget(self, operation, context: _RunContext): + remaining = context.deadline - time.monotonic() + return await asyncio.wait_for(operation, timeout=remaining) + + async def _run(self, workspace, plan: ExecutionPlan): + spec = WorkspaceRunProgramSpec( + cmd=plan.argv[0], + args=list(plan.argv[1:]), + env=dict(plan.environment), + cwd=plan.cwd, + timeout=plan.timeout_seconds, + limits=WorkspaceResourceLimits( + cpu_percent=CPU_PERCENT_LIMIT, + memory_mb=MEMORY_LIMIT_MB, + max_pids=PROCESS_LIMIT, + ), + ) + return await self._handle.require_runtime().runner().run_program(workspace, spec) + + async def _collect(self, workspace, plan: ExecutionPlan) -> str: + if self._handle.kind == "local": + return self._collect_local(workspace.path, plan) + files = await self._handle.require_runtime().fs().collect( + workspace, + [plan.output_path], + ) + if not files: + return "" + output = files[0] + if output.truncated or output.size_bytes > plan.output_limit_bytes: + raise ValueError("sandbox output exceeded configured limit") + return _truncate(output.content, plan.output_limit_bytes) + + @staticmethod + def _collect_local(workspace_path: str, plan: ExecutionPlan) -> str: + root = Path(workspace_path).resolve() + output = (root / plan.output_path).resolve() + if root not in output.parents or output.is_symlink(): + raise ValueError("sandbox output escaped workspace") + size = output.stat().st_size + if size > plan.output_limit_bytes: + raise ValueError("sandbox output exceeded configured limit") + return output.read_text(encoding="utf-8") + + def _normalize_result(self, result, output: str, context: _RunContext) -> SandboxExecution: + status = SandboxStatus.SUCCEEDED + if result.timed_out: + status = SandboxStatus.TIMED_OUT + elif result.exit_code: + status = SandboxStatus.FAILED + limit = min(context.plan.output_limit_bytes, MAX_OUTPUT_BYTES) + stdout, stderr, output = self._bounded_outputs( + self._redactor.redact_text(result.stdout), + self._redactor.redact_text(result.stderr), + self._redactor.redact_text(output), + limit, + ) + run = SandboxRun( + status=status, + exit_code=result.exit_code, + timed_out=result.timed_out, + duration_ms=int((time.monotonic() - context.started) * MILLISECONDS_PER_SECOND), + stdout=stdout, + stderr=stderr, + error_type=None if status == SandboxStatus.SUCCEEDED else "ProgramError", + ) + return SandboxExecution(run=run, output=output) + + @staticmethod + def _bounded_outputs( + stdout: str, + stderr: str, + output: str, + limit: int, + ) -> tuple[str, str, str]: + bounded = [] + remaining = limit + for value in (stdout, stderr, output): + item = _truncate(value, remaining) + bounded.append(item) + remaining = max(0, remaining - len(item.encode("utf-8"))) + return bounded[0], bounded[1], bounded[2] + + def _failure( + self, + status: SandboxStatus, + error: Exception, + context: _RunContext, + ) -> SandboxExecution: + limit = min(context.plan.output_limit_bytes, MAX_OUTPUT_BYTES) + run = SandboxRun( + status=status, + timed_out=status == SandboxStatus.TIMED_OUT, + duration_ms=int((time.monotonic() - context.started) * MILLISECONDS_PER_SECOND), + stderr=_truncate(self._redactor.redact_text(str(error)), limit), + error_type=type(error).__name__, + ) + return SandboxExecution(run=run, output="") + + async def _cleanup(self, state: _ExecutionState) -> Exception | None: + if state.workspace is None: + return None + try: + await asyncio.wait_for( + self._handle.require_runtime().manager().cleanup(state.exec_id), + timeout=CLEANUP_TIMEOUT_SECONDS, + ) + except Exception as exc: # pylint: disable=broad-except + return exc + return None 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 000000000..79d601c69 --- /dev/null +++ b/examples/skills_code_review_agent/agent/storage.py @@ -0,0 +1,444 @@ +"""SQL schema and storage facade for code review tasks.""" + +from __future__ import annotations + +from datetime import datetime +from datetime import timezone +import hashlib +from typing import Any +import uuid + +from sqlalchemy.exc import IntegrityError +from sqlalchemy import Float +from sqlalchemy import ForeignKey +from sqlalchemy import Index +from sqlalchemy import Integer +from sqlalchemy import String +from sqlalchemy import Text +from sqlalchemy import UniqueConstraint +from sqlalchemy.orm import DeclarativeBase +from sqlalchemy.orm import Mapped +from sqlalchemy.orm import mapped_column +from trpc_agent_sdk.storage import DynamicJSON +from trpc_agent_sdk.storage import SqlAsyncContextManager +from trpc_agent_sdk.storage import SqlCondition +from trpc_agent_sdk.storage import SqlKey +from trpc_agent_sdk.storage import SqlStorage + +from .constants import ASYNC_SQL_DRIVER_MARKERS +from .constants import DB_CATEGORY_LENGTH +from .constants import DB_ID_LENGTH +from .constants import DB_PATH_LENGTH +from .constants import DB_SOURCE_LENGTH +from .constants import DB_STATUS_LENGTH +from .constants import DB_TITLE_LENGTH +from .constants import SQLITE_BUSY_TIMEOUT_MILLISECONDS +from .constants import MAX_TEXT_FIELD_LENGTH +from .models import FilterDecision +from .models import Finding +from .models import ReviewInput +from .models import ReviewReport +from .models import SandboxRun +from .models import TaskStatus +from .policy import SecretRedactor + +_SEVERITY_RANK = { + "warning": 0, + "low": 1, + "medium": 2, + "high": 3, + "critical": 4, +} +_MERGE_SEPARATOR = " | " + + +def utc_now() -> datetime: + """Return one timezone-aware UTC timestamp.""" + return datetime.now(timezone.utc) + + +class ReviewStorageBase(DeclarativeBase): + """Metadata root dedicated to this example.""" + + +class ReviewTaskRow(ReviewStorageBase): + """Top-level review task.""" + + __tablename__ = "review_tasks" + + id: Mapped[str] = mapped_column(String(DB_ID_LENGTH), primary_key=True) + status: Mapped[str] = mapped_column(String(DB_STATUS_LENGTH), nullable=False) + input_kind: Mapped[str] = mapped_column(String(DB_STATUS_LENGTH), nullable=False) + input_digest: Mapped[str] = mapped_column(String(DB_ID_LENGTH), nullable=False) + input_summary: Mapped[dict[str, Any]] = mapped_column(DynamicJSON, nullable=False) + conclusion: Mapped[str | None] = mapped_column(Text) + failure_reason: Mapped[str | None] = mapped_column(Text) + started_at: Mapped[datetime] = mapped_column(default=utc_now, nullable=False) + finished_at: Mapped[datetime | None] = mapped_column() + + +class FilterDecisionRow(ReviewStorageBase): + """Persisted decision made before sandbox execution.""" + + __tablename__ = "review_filter_decisions" + + id: Mapped[str] = mapped_column(String(DB_ID_LENGTH), primary_key=True) + task_id: Mapped[str] = mapped_column( + ForeignKey("review_tasks.id", ondelete="CASCADE"), + index=True, + nullable=False, + ) + action: Mapped[str] = mapped_column(String(DB_STATUS_LENGTH), nullable=False) + rule_id: Mapped[str] = mapped_column(String(DB_CATEGORY_LENGTH), nullable=False) + reason: Mapped[str] = mapped_column(Text, nullable=False) + plan_digest: Mapped[str] = mapped_column(String(DB_ID_LENGTH), nullable=False) + created_at: Mapped[datetime] = mapped_column(default=utc_now, nullable=False) + + +class SandboxRunRow(ReviewStorageBase): + """One sandbox attempt, including failures and timeouts.""" + + __tablename__ = "review_sandbox_runs" + + id: Mapped[str] = mapped_column(String(DB_ID_LENGTH), primary_key=True) + task_id: Mapped[str] = mapped_column( + ForeignKey("review_tasks.id", ondelete="CASCADE"), + index=True, + nullable=False, + ) + status: Mapped[str] = mapped_column(String(DB_STATUS_LENGTH), nullable=False) + exit_code: Mapped[int | None] = mapped_column(Integer) + timed_out: Mapped[bool] = mapped_column(default=False, nullable=False) + duration_ms: Mapped[int] = mapped_column(Integer, nullable=False) + stdout: Mapped[str] = mapped_column(Text, default="", nullable=False) + stderr: Mapped[str] = mapped_column(Text, default="", nullable=False) + error_type: Mapped[str | None] = mapped_column(String(DB_CATEGORY_LENGTH)) + created_at: Mapped[datetime] = mapped_column(default=utc_now, nullable=False) + + +class FindingRow(ReviewStorageBase): + """One normalized and deduplicated code-review finding.""" + + __tablename__ = "review_findings" + __table_args__ = ( + UniqueConstraint("task_id", "fingerprint", name="uq_review_finding_fingerprint"), + Index( + "ix_review_finding_location", + "task_id", + "file_path", + "line", + "category", + ), + ) + + id: Mapped[str] = mapped_column(String(DB_ID_LENGTH), primary_key=True) + task_id: Mapped[str] = mapped_column( + ForeignKey("review_tasks.id", ondelete="CASCADE"), + index=True, + nullable=False, + ) + fingerprint: Mapped[str] = mapped_column(String(DB_ID_LENGTH), nullable=False) + severity: Mapped[str] = mapped_column(String(DB_STATUS_LENGTH), nullable=False) + category: Mapped[str] = mapped_column(String(DB_CATEGORY_LENGTH), nullable=False) + file_path: Mapped[str] = mapped_column(String(DB_PATH_LENGTH), nullable=False) + line: Mapped[int | None] = mapped_column(Integer) + title: Mapped[str] = mapped_column(String(DB_TITLE_LENGTH), nullable=False) + evidence: Mapped[str] = mapped_column(Text, nullable=False) + recommendation: Mapped[str] = mapped_column(Text, nullable=False) + confidence: Mapped[float] = mapped_column(Float, nullable=False) + source: Mapped[str] = mapped_column(String(DB_SOURCE_LENGTH), nullable=False) + needs_human_review: Mapped[bool] = mapped_column(default=False, nullable=False) + created_at: Mapped[datetime] = mapped_column(default=utc_now, nullable=False) + + +class ReviewReportRow(ReviewStorageBase): + """Generated JSON and Markdown report.""" + + __tablename__ = "review_reports" + + task_id: Mapped[str] = mapped_column( + ForeignKey("review_tasks.id", ondelete="CASCADE"), + primary_key=True, + ) + report_json: Mapped[dict[str, Any]] = mapped_column(DynamicJSON, nullable=False) + report_markdown: Mapped[str] = mapped_column(Text, nullable=False) + metrics: Mapped[dict[str, Any]] = mapped_column(DynamicJSON, nullable=False) + created_at: Mapped[datetime] = mapped_column(default=utc_now, nullable=False) + + +def create_sql_storage(db_url: str) -> SqlStorage: + """Create storage with metadata limited to review tables.""" + return SqlStorage( + is_async=any(marker in db_url for marker in ASYNC_SQL_DRIVER_MARKERS), + db_url=db_url, + metadata=ReviewStorageBase.metadata, + ) + + +def finding_fingerprint(finding: Finding) -> str: + """Build the location/category fingerprint required for deduplication.""" + value = f"{finding.file}\x00{finding.line}\x00{finding.category.value}" + return hashlib.sha256(value.encode("utf-8")).hexdigest() + + +class ReviewStore: + """Narrow async facade over the repository SQL storage.""" + + def __init__(self, db_url: str, redactor: SecretRedactor) -> None: + self._db_url = db_url + self._storage = create_sql_storage(db_url) + self._redactor = redactor + + async def initialize(self) -> None: + """Create missing tables and configure SQLite concurrency.""" + await self._storage.create_sql_engine() + if self._db_url.startswith("sqlite"): + await self._configure_sqlite() + + async def close(self) -> None: + """Close the underlying SQL engine.""" + await self._storage.close() + + async def _configure_sqlite(self) -> None: + from sqlalchemy import text + + async with SqlAsyncContextManager(self._storage) as session: + await self._execute_sql(session, text("PRAGMA journal_mode=WAL")) + statement = text(f"PRAGMA busy_timeout={SQLITE_BUSY_TIMEOUT_MILLISECONDS}") + await self._execute_sql(session, statement) + await self._storage.commit(session) + + @staticmethod + async def _execute_sql(session, statement): + from sqlalchemy.ext.asyncio import AsyncSession + + if isinstance(session, AsyncSession): + return await session.execute(statement) + return session.execute(statement) + + async def create_task(self, task_id: str, review_input: ReviewInput) -> None: + """Persist a CREATED task without raw diff content.""" + summary = { + "source": review_input.source, + "file_count": len(review_input.files), + "warnings": review_input.warnings, + } + row = ReviewTaskRow( + id=task_id, + status=TaskStatus.CREATED.value, + input_kind=review_input.kind.value, + input_digest=review_input.digest, + input_summary=self._redactor.redact_value(summary), + ) + await self._add_and_commit(row) + + async def save_filter_decisions( + self, + task_id: str, + decisions: list[FilterDecision], + ) -> None: + """Commit Filter audit rows before execution.""" + rows = [ + FilterDecisionRow( + id=uuid.uuid4().hex, + task_id=task_id, + action=item.action.value, + rule_id=item.rule_id, + reason=self._redactor.redact_text(item.reason), + plan_digest=item.plan_digest, + created_at=item.created_at, + ) for item in decisions + ] + await self._add_many_and_commit(rows) + + async def save_sandbox_run(self, task_id: str, run: SandboxRun) -> None: + """Persist one bounded and redacted sandbox record.""" + row = SandboxRunRow( + id=uuid.uuid4().hex, + task_id=task_id, + status=run.status.value, + exit_code=run.exit_code, + timed_out=run.timed_out, + duration_ms=run.duration_ms, + stdout=self._redactor.redact_text(run.stdout), + stderr=self._redactor.redact_text(run.stderr), + error_type=self._redactor.redact_text(run.error_type or "") or None, + ) + await self._add_and_commit(row) + + async def save_findings( + self, + task_id: str, + findings: list[Finding], + human_review: set[str], + ) -> None: + """Insert deduplicated findings; tolerate concurrent duplicate writes.""" + for finding in findings: + await self._save_finding(task_id, finding, human_review) + + async def _save_finding( + self, + task_id: str, + finding: Finding, + human_review: set[str], + ) -> None: + fingerprint = finding_fingerprint(finding) + row = FindingRow( + id=uuid.uuid4().hex, + task_id=task_id, + fingerprint=fingerprint, + severity=finding.severity.value, + category=finding.category.value, + file_path=finding.file, + line=finding.line, + title=self._redactor.redact_text(finding.title), + evidence=self._redactor.redact_text(finding.evidence), + recommendation=self._redactor.redact_text(finding.recommendation), + confidence=finding.confidence, + source=self._redactor.redact_text(finding.source), + needs_human_review=fingerprint in human_review, + ) + try: + await self._add_and_commit(row) + except IntegrityError: + await self._merge_existing_finding(task_id, row) + + async def complete_task(self, report: ReviewReport, markdown: str) -> None: + """Atomically persist report and terminal task state.""" + payload = self._redactor.redact_value(report.model_dump(mode="json")) + async with SqlAsyncContextManager(self._storage) as session: + task = await self._storage.get( + session, + SqlKey((report.task_id, ), ReviewTaskRow), + ) + if task is None: + raise KeyError(f"unknown review task: {report.task_id}") + row = ReviewReportRow( + task_id=report.task_id, + report_json=payload, + report_markdown=self._redactor.redact_text(markdown), + metrics=payload["metrics"], + created_at=report.created_at, + ) + task.status = report.status.value + task.conclusion = self._redactor.redact_text(report.conclusion) + task.finished_at = utc_now() + await self._storage.add(session, row) + await self._storage.commit(session) + + async def update_status( + self, + task_id: str, + status: TaskStatus, + failure: str | None = None, + ) -> None: + """Update task state and terminal timestamps.""" + async with SqlAsyncContextManager(self._storage) as session: + row = await self._storage.get(session, SqlKey((task_id, ), ReviewTaskRow)) + if row is None: + raise KeyError(f"unknown review task: {task_id}") + row.status = status.value + if status == TaskStatus.FAILED and failure: + row.conclusion = "Review task failed." + row.failure_reason = self._redactor.redact_text(failure or "") or None + if status in { + TaskStatus.COMPLETE, + TaskStatus.PARTIAL, + TaskStatus.FAILED, + TaskStatus.DENIED, + }: + row.finished_at = utc_now() + await self._storage.commit(session) + + async def _merge_existing_finding( + self, + task_id: str, + incoming: FindingRow, + ) -> None: + condition = SqlCondition( + filters=[ + FindingRow.task_id == task_id, + FindingRow.fingerprint == incoming.fingerprint, + ], + limit=1, + ) + async with SqlAsyncContextManager(self._storage) as session: + rows = await self._storage.query( + session, + SqlKey((), FindingRow), + condition, + ) + if not rows: + raise RuntimeError("finding conflict could not be reloaded") + existing = rows[0] + if _incoming_finding_wins(existing, incoming): + existing.severity = incoming.severity + existing.title = incoming.title + existing.recommendation = incoming.recommendation + existing.confidence = incoming.confidence + existing.evidence = _merge_db_text( + existing.evidence, + incoming.evidence, + MAX_TEXT_FIELD_LENGTH, + ) + existing.source = _merge_db_text( + existing.source, + incoming.source, + DB_SOURCE_LENGTH, + ) + existing.needs_human_review |= incoming.needs_human_review + await self._storage.commit(session) + + async def get_report(self, task_id: str) -> dict[str, Any] | None: + """Return a task and its persisted report by id.""" + async with SqlAsyncContextManager(self._storage) as session: + task = await self._storage.get(session, SqlKey((task_id, ), ReviewTaskRow)) + if task is None: + return None + report = await self._storage.get(session, SqlKey((task_id, ), ReviewReportRow)) + return { + "task_id": task.id, + "status": task.status, + "input_summary": task.input_summary, + "failure_reason": task.failure_reason, + "report": report.report_json if report else None, + "markdown": report.report_markdown if report else None, + } + + async def list_findings(self, task_id: str) -> list[FindingRow]: + """Query findings for tests and integrations.""" + condition = SqlCondition( + filters=[FindingRow.task_id == task_id], + order_func=lambda: FindingRow.id, + ) + async with SqlAsyncContextManager(self._storage) as session: + return await self._storage.query( + session, + SqlKey((), FindingRow), + condition, + ) + + async def _add_and_commit(self, row: ReviewStorageBase) -> None: + async with SqlAsyncContextManager(self._storage) as session: + await self._storage.add(session, row) + await self._storage.commit(session) + + async def _add_many_and_commit(self, rows: list[ReviewStorageBase]) -> None: + async with SqlAsyncContextManager(self._storage) as session: + for row in rows: + await self._storage.add(session, row) + await self._storage.commit(session) + + +def _incoming_finding_wins(existing: FindingRow, incoming: FindingRow) -> bool: + current = (_SEVERITY_RANK[existing.severity], existing.confidence) + candidate = (_SEVERITY_RANK[incoming.severity], incoming.confidence) + return candidate > current + + +def _merge_db_text(left: str, right: str, limit: int) -> str: + values = [] + for value in (left, right): + if value and value not in values: + values.append(value) + return _MERGE_SEPARATOR.join(values)[:limit] diff --git a/examples/skills_code_review_agent/fixtures/async_resource.diff b/examples/skills_code_review_agent/fixtures/async_resource.diff new file mode 100644 index 000000000..6895337b4 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/async_resource.diff @@ -0,0 +1,12 @@ +diff --git a/app/worker.py b/app/worker.py +index 1111111..2222222 100644 +--- a/app/worker.py ++++ b/app/worker.py +@@ -1,2 +1,6 @@ + async def run_job(path): +- return await execute_async(path) ++ time.sleep(1) ++ handle = open(path) ++ result = execute_async(handle.read()) ++ return result ++ diff --git a/examples/skills_code_review_agent/fixtures/async_resource.expected.json b/examples/skills_code_review_agent/fixtures/async_resource.expected.json new file mode 100644 index 000000000..b525765ed --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/async_resource.expected.json @@ -0,0 +1,27 @@ +{ + "name": "async_resource", + "expected_findings": [ + { + "category": "async_error", + "file": "app/worker.py", + "line": 2 + }, + { + "category": "resource_leak", + "file": "app/worker.py", + "line": 3 + }, + { + "category": "async_error", + "file": "app/worker.py", + "line": 4 + }, + { + "category": "missing_test", + "file": "app/worker.py", + "line": 2 + } + ], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} 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 000000000..63866b1de --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/clean.diff @@ -0,0 +1,32 @@ +diff --git a/app/math_utils.py b/app/math_utils.py +index 1111111..2222222 100644 +--- a/app/math_utils.py ++++ b/app/math_utils.py +@@ -1,2 +1,5 @@ + def add(left, right): +- return left + right ++ result = left + right ++ return result ++ ++ +diff --git a/tests/test_math_utils.py b/tests/test_math_utils.py +index 3333333..4444444 100644 +--- a/tests/test_math_utils.py ++++ b/tests/test_math_utils.py +@@ -1,2 +1,5 @@ + def test_add(): +- assert add(1, 2) == 3 ++ result = add(1, 2) ++ ++ assert result == 3 ++ +diff --git a/app/storage.py b/app/storage.py +new file mode 100644 +index 0000000..5555555 +--- /dev/null ++++ b/app/storage.py +@@ -0,0 +1,4 @@ ++def load(db, path): ++ api_key = os.getenv("API_KEY") ++ with open(path) as handle: ++ return db.execute("SELECT * FROM users WHERE id = ?", (handle.read(),)) diff --git a/examples/skills_code_review_agent/fixtures/clean.expected.json b/examples/skills_code_review_agent/fixtures/clean.expected.json new file mode 100644 index 000000000..61bd1f2e0 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/clean.expected.json @@ -0,0 +1,6 @@ +{ + "name": "clean", + "expected_findings": [], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} diff --git a/examples/skills_code_review_agent/fixtures/db_lifecycle.diff b/examples/skills_code_review_agent/fixtures/db_lifecycle.diff new file mode 100644 index 000000000..92bcce290 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/db_lifecycle.diff @@ -0,0 +1,11 @@ +diff --git a/app/repository.py b/app/repository.py +index 1111111..2222222 100644 +--- a/app/repository.py ++++ b/app/repository.py +@@ -1,2 +1,5 @@ + def load_user(engine, user_id): +- return engine.execute(QUERY, {"id": user_id}).one() ++ connection = engine.connect() ++ cursor = connection.cursor() ++ cursor.execute(QUERY, {"id": user_id}) ++ return cursor.fetchone() diff --git a/examples/skills_code_review_agent/fixtures/db_lifecycle.expected.json b/examples/skills_code_review_agent/fixtures/db_lifecycle.expected.json new file mode 100644 index 000000000..c57cde304 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/db_lifecycle.expected.json @@ -0,0 +1,22 @@ +{ + "name": "db_lifecycle", + "expected_findings": [ + { + "category": "db_lifecycle", + "file": "app/repository.py", + "line": 2 + }, + { + "category": "db_lifecycle", + "file": "app/repository.py", + "line": 3 + }, + { + "category": "missing_test", + "file": "app/repository.py", + "line": 2 + } + ], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} 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 000000000..23bec7cc5 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/duplicate.diff @@ -0,0 +1,10 @@ +diff --git a/app/template.py b/app/template.py +index 1111111..2222222 100644 +--- a/app/template.py ++++ b/app/template.py +@@ -1,2 +1,4 @@ + def render(expression): +- return expression ++ result = eval(expression); subprocess.run(expression, shell=True) ++ return result ++ diff --git a/examples/skills_code_review_agent/fixtures/duplicate.expected.json b/examples/skills_code_review_agent/fixtures/duplicate.expected.json new file mode 100644 index 000000000..b63365927 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/duplicate.expected.json @@ -0,0 +1,17 @@ +{ + "name": "duplicate", + "expected_findings": [ + { + "category": "security", + "file": "app/template.py", + "line": 2 + }, + { + "category": "missing_test", + "file": "app/template.py", + "line": 2 + } + ], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} diff --git a/examples/skills_code_review_agent/fixtures/missing_tests.diff b/examples/skills_code_review_agent/fixtures/missing_tests.diff new file mode 100644 index 000000000..86a53434a --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/missing_tests.diff @@ -0,0 +1,11 @@ +diff --git a/app/pricing.py b/app/pricing.py +index 1111111..2222222 100644 +--- a/app/pricing.py ++++ b/app/pricing.py +@@ -1,2 +1,5 @@ + def total(price, quantity): +- return price * quantity ++ if quantity < 0: ++ raise ValueError("quantity must be non-negative") ++ return round(price * quantity, 2) ++ diff --git a/examples/skills_code_review_agent/fixtures/missing_tests.expected.json b/examples/skills_code_review_agent/fixtures/missing_tests.expected.json new file mode 100644 index 000000000..bc5727712 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/missing_tests.expected.json @@ -0,0 +1,12 @@ +{ + "name": "missing_tests", + "expected_findings": [ + { + "category": "missing_test", + "file": "app/pricing.py", + "line": 2 + } + ], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} diff --git a/examples/skills_code_review_agent/fixtures/sandbox_failure.diff b/examples/skills_code_review_agent/fixtures/sandbox_failure.diff new file mode 100644 index 000000000..798424b92 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/sandbox_failure.diff @@ -0,0 +1,10 @@ +diff --git a/app/status.py b/app/status.py +index 1111111..2222222 100644 +--- a/app/status.py ++++ b/app/status.py +@@ -1,2 +1,4 @@ + def status(): +- return "unknown" ++ # REVIEW_FIXTURE:TIMEOUT ++ return "ready" ++ diff --git a/examples/skills_code_review_agent/fixtures/sandbox_failure.expected.json b/examples/skills_code_review_agent/fixtures/sandbox_failure.expected.json new file mode 100644 index 000000000..f1582cb8b --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/sandbox_failure.expected.json @@ -0,0 +1,6 @@ +{ + "name": "sandbox_failure", + "expected_findings": [], + "expected_sandbox_status": "timed_out", + "max_false_positives": 0 +} diff --git a/examples/skills_code_review_agent/fixtures/secrets.diff b/examples/skills_code_review_agent/fixtures/secrets.diff new file mode 100644 index 000000000..b6c7144e0 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/secrets.diff @@ -0,0 +1,10 @@ +diff --git a/app/settings.py b/app/settings.py +index 1111111..2222222 100644 +--- a/app/settings.py ++++ b/app/settings.py +@@ -1,2 +1,4 @@ + def service_config(): +- return {"endpoint": "https://service.invalid"} ++ api_key = "synthetic-secret-value-12345" ++ return {"endpoint": "https://service.invalid", "api_key": api_key} ++ diff --git a/examples/skills_code_review_agent/fixtures/secrets.expected.json b/examples/skills_code_review_agent/fixtures/secrets.expected.json new file mode 100644 index 000000000..4f1d613e3 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/secrets.expected.json @@ -0,0 +1,17 @@ +{ + "name": "secrets", + "expected_findings": [ + { + "category": "secret_leak", + "file": "app/settings.py", + "line": 2 + }, + { + "category": "missing_test", + "file": "app/settings.py", + "line": 3 + } + ], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} 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 000000000..9289efa14 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/security.diff @@ -0,0 +1,11 @@ +diff --git a/app/runner.py b/app/runner.py +index 1111111..2222222 100644 +--- a/app/runner.py ++++ b/app/runner.py +@@ -1,4 +1,5 @@ + import subprocess + + def run(command): +- return subprocess.run(["tool", command], check=True) ++ return subprocess.run(command, shell=True, check=True) ++ diff --git a/examples/skills_code_review_agent/fixtures/security.expected.json b/examples/skills_code_review_agent/fixtures/security.expected.json new file mode 100644 index 000000000..b5c5292d2 --- /dev/null +++ b/examples/skills_code_review_agent/fixtures/security.expected.json @@ -0,0 +1,17 @@ +{ + "name": "security", + "expected_findings": [ + { + "category": "security", + "file": "app/runner.py", + "line": 4 + }, + { + "category": "missing_test", + "file": "app/runner.py", + "line": 4 + } + ], + "expected_sandbox_status": "succeeded", + "max_false_positives": 0 +} diff --git a/examples/skills_code_review_agent/reports/review_report.json b/examples/skills_code_review_agent/reports/review_report.json new file mode 100644 index 000000000..919feeabd --- /dev/null +++ b/examples/skills_code_review_agent/reports/review_report.json @@ -0,0 +1,112 @@ +{ + "conclusion": "2 actionable findings and 0 warnings.", + "created_at": "2026-07-26T13:25:53.702830Z", + "failures": [], + "filter_decisions": [ + { + "action": "allow", + "created_at": "2026-07-26T13:25:53.532555Z", + "plan_digest": "ff6e18b8ee75bdf21791076f077401a7307981737e2ca2b97a614e01f8a633ff", + "reason": "plan digest verified", + "rule_id": "plan.integrity" + }, + { + "action": "allow", + "created_at": "2026-07-26T13:25:53.532568Z", + "plan_digest": "ff6e18b8ee75bdf21791076f077401a7307981737e2ca2b97a614e01f8a633ff", + "reason": "fixed scanner argv", + "rule_id": "command.allowlist" + }, + { + "action": "allow", + "created_at": "2026-07-26T13:25:53.532632Z", + "plan_digest": "ff6e18b8ee75bdf21791076f077401a7307981737e2ca2b97a614e01f8a633ff", + "reason": "workspace-relative paths", + "rule_id": "path.workspace" + }, + { + "action": "allow", + "created_at": "2026-07-26T13:25:53.532637Z", + "plan_digest": "ff6e18b8ee75bdf21791076f077401a7307981737e2ca2b97a614e01f8a633ff", + "reason": "environment allowlist", + "rule_id": "environment.allowlist" + }, + { + "action": "allow", + "created_at": "2026-07-26T13:25:53.532645Z", + "plan_digest": "ff6e18b8ee75bdf21791076f077401a7307981737e2ca2b97a614e01f8a633ff", + "reason": "unsafe local runtime explicitly enabled for development", + "rule_id": "runtime.isolation" + }, + { + "action": "allow", + "created_at": "2026-07-26T13:25:53.532650Z", + "plan_digest": "ff6e18b8ee75bdf21791076f077401a7307981737e2ca2b97a614e01f8a633ff", + "reason": "execution budget accepted", + "rule_id": "budget.limit" + } + ], + "findings": [ + { + "category": "missing_test", + "confidence": 0.76, + "evidence": "app/runner.py", + "file": "app/runner.py", + "line": 4, + "recommendation": "Add a focused test covering the changed behavior and failure path.", + "severity": "medium", + "source": "skill:code-review/tests.missing", + "title": "Production change has no test update" + }, + { + "category": "security", + "confidence": 0.98, + "evidence": " return subprocess.run(command, shell=True, check=True)", + "file": "app/runner.py", + "line": 4, + "recommendation": "Pass a fixed argv list and keep shell execution disabled.", + "severity": "critical", + "source": "skill:code-review/security.shell-true", + "title": "Shell execution enabled" + } + ], + "input_summary": { + "digest": "ac238e8ec4fb5fbdfecbae29d6a723b56da9bb179abc078823727291d3428abb", + "file_count": 1, + "kind": "fixture" + }, + "metrics": { + "blocked_count": 0, + "exceptions_by_type": {}, + "filter_actions": { + "allow": 6 + }, + "finding_count": 2, + "findings_by_category": { + "missing_test": 1, + "security": 1 + }, + "findings_by_severity": { + "critical": 1, + "medium": 1 + }, + "sandbox_duration_ms": 150, + "tool_calls": 2, + "total_duration_ms": 2789 + }, + "needs_human_review": [], + "sandbox_runs": [ + { + "duration_ms": 150, + "error_type": null, + "exit_code": 0, + "status": "succeeded", + "stderr": "", + "stdout": "findings_written=2\r\n", + "timed_out": false + } + ], + "status": "complete", + "task_id": "3f0730ca180f4ff2abd8472a871fb369", + "warnings": [] +} diff --git a/examples/skills_code_review_agent/reports/review_report.md b/examples/skills_code_review_agent/reports/review_report.md new file mode 100644 index 000000000..f9cfcc8e3 --- /dev/null +++ b/examples/skills_code_review_agent/reports/review_report.md @@ -0,0 +1,66 @@ +# Code Review Report: 3f0730ca180f4ff2abd8472a871fb369 + +## Task summary + +- Status: `complete` +- Files: 1 +- Conclusion: 2 actionable findings and 0 warnings. + +## Findings + +- **medium / missing_test** `app/runner.py:4` — Production change has no test update + Evidence: app/runner.py + Recommendation: Add a focused test covering the changed behavior and failure path. + Confidence: 0.76; Source: `skill:code-review/tests.missing` +- **critical / security** `app/runner.py:4` — Shell execution enabled + Evidence: return subprocess.run(command, shell=True, check=True) + Recommendation: Pass a fixed argv list and keep shell execution disabled. + Confidence: 0.98; Source: `skill:code-review/security.shell-true` + +## Warnings / needs human review + +- None + +## Filter decisions + +- `allow` `plan.integrity`: plan digest verified +- `allow` `command.allowlist`: fixed scanner argv +- `allow` `path.workspace`: workspace-relative paths +- `allow` `environment.allowlist`: environment allowlist +- `allow` `runtime.isolation`: unsafe local runtime explicitly enabled for development +- `allow` `budget.limit`: execution budget accepted + +## Sandbox runs + +- `succeeded` exit=0 timeout=False duration_ms=150 + +## Exceptions + + +## Metrics + +```json +{ + "blocked_count": 0, + "exceptions_by_type": {}, + "filter_actions": { + "allow": 6 + }, + "finding_count": 2, + "findings_by_category": { + "missing_test": 1, + "security": 1 + }, + "findings_by_severity": { + "critical": 1, + "medium": 1 + }, + "sandbox_duration_ms": 150, + "tool_calls": 2, + "total_duration_ms": 2789 +} +``` + +## Final conclusion + +2 actionable findings and 0 warnings. 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 000000000..1232ad176 --- /dev/null +++ b/examples/skills_code_review_agent/run_agent.py @@ -0,0 +1,173 @@ +#!/usr/bin/env python3 +"""Run or query the Skills code-review Agent example.""" + +from __future__ import annotations + +import argparse +import asyncio +import json +import os +from pathlib import Path + +from trpc_agent_sdk.models import OpenAIModel + +from agent.constants import DB_URL_ENV +from agent.constants import DEFAULT_DB_URL +from agent.constants import DEFAULT_OUTPUT_DIR +from agent.input_parser import GitDiffOptions +from agent.input_parser import InputValidationError +from agent.input_parser import load_diff_file +from agent.input_parser import load_file_list +from agent.input_parser import load_repo_diff +from agent.models import InputKind +from agent.pipeline import FAKE_MODEL_NAME +from agent.pipeline import FakeReviewModel +from agent.pipeline import PipelineDependencies +from agent.pipeline import ReviewPipeline +from agent.policy import SecretRedactor +from agent.sandbox import create_runtime +from agent.sandbox import create_plan_runtime +from agent.storage import ReviewStore + +EXAMPLE_ROOT = Path(__file__).resolve().parent +FIXTURE_ROOT = EXAMPLE_ROOT / "fixtures" +SKILL_ROOT = EXAMPLE_ROOT / "skills" +DEFAULT_MODEL_NAME = "deepseek-chat" +MODEL_NAME_ENV = "MODEL_NAME" +API_KEY_ENV = "OPENAI_API_KEY" +BASE_URL_ENV = "OPENAI_BASE_URL" + + +def _add_run_arguments(parser: argparse.ArgumentParser) -> None: + inputs = parser.add_mutually_exclusive_group(required=True) + inputs.add_argument("--diff-file", type=Path) + inputs.add_argument("--file-list", type=Path) + inputs.add_argument("--repo-path", type=Path) + inputs.add_argument("--fixture") + git_mode = parser.add_mutually_exclusive_group() + git_mode.add_argument("--staged", action="store_true") + git_mode.add_argument("--worktree", action="store_true") + parser.add_argument("--base") + parser.add_argument("--head") + parser.add_argument("--runtime", choices=("container", "local"), default="container") + parser.add_argument("--fake-model", action="store_true") + parser.add_argument("--dry-run", action="store_true") + parser.add_argument("--db-url", default=os.getenv(DB_URL_ENV, DEFAULT_DB_URL)) + parser.add_argument("--output-dir", type=Path, default=DEFAULT_OUTPUT_DIR) + parser.add_argument("--model-name", default=os.getenv(MODEL_NAME_ENV, DEFAULT_MODEL_NAME)) + parser.add_argument("--api-key", default=os.getenv(API_KEY_ENV)) + parser.add_argument("--base-url", default=os.getenv(BASE_URL_ENV)) + + +def parse_args() -> argparse.Namespace: + """Parse run/show subcommands.""" + parser = argparse.ArgumentParser(description=__doc__) + commands = parser.add_subparsers(dest="command", required=True) + _add_run_arguments(commands.add_parser("run", help="run a review")) + show = commands.add_parser("show", help="query a persisted task") + show.add_argument("--task-id", required=True) + show.add_argument("--db-url", default=os.getenv(DB_URL_ENV, DEFAULT_DB_URL)) + return parser.parse_args() + + +def _fixture_path(name: str) -> Path: + if not name or Path(name).name != name: + raise InputValidationError("fixture must be a simple name") + path = FIXTURE_ROOT / f"{name}.diff" + if not path.is_file(): + raise InputValidationError(f"unknown fixture: {name}") + return path + + +def _load_input(args: argparse.Namespace): + has_git_options = args.staged or args.worktree or args.base or args.head + if has_git_options and not args.repo_path: + raise InputValidationError("Git range options require --repo-path") + if args.diff_file: + return load_diff_file(args.diff_file) + if args.file_list: + return load_file_list(args.file_list) + if args.repo_path: + options = GitDiffOptions( + staged=args.staged, + worktree=args.worktree, + base=args.base, + head=args.head, + ) + return load_repo_diff(args.repo_path, options) + return load_diff_file(_fixture_path(args.fixture)).model_copy(update={ + "kind": InputKind.FIXTURE, + "source": args.fixture + }, ) + + +def _create_model(args: argparse.Namespace): + if args.fake_model or args.dry_run: + return FakeReviewModel(model_name=FAKE_MODEL_NAME) + if not args.api_key: + raise ValueError("real model mode requires --api-key or OPENAI_API_KEY") + return OpenAIModel( + model_name=args.model_name, + api_key=args.api_key, + base_url=args.base_url, + ) + + +async def _run(args: argparse.Namespace) -> int: + redactor = SecretRedactor() + store = ReviewStore(args.db_url, redactor) + await store.initialize() + try: + dependencies = PipelineDependencies( + store=store, + runtime=(create_plan_runtime(args.runtime) if args.dry_run else create_runtime(args.runtime)), + skill_root=SKILL_ROOT, + output_dir=args.output_dir, + model=_create_model(args), + ) + report, json_path, markdown_path = await ReviewPipeline(dependencies).run( + _load_input(args), + dry_run=args.dry_run, + ) + print(f"task_id={report.task_id}") + print(f"status={report.status.value}") + print(f"json_report={json_path}") + print(f"markdown_report={markdown_path}") + return 0 if report.status.value in {"complete", "partial"} else 1 + finally: + await store.close() + + +async def _show(args: argparse.Namespace) -> int: + store = ReviewStore(args.db_url, SecretRedactor()) + await store.initialize() + try: + result = await store.get_report(args.task_id) + if result is None: + print("task not found") + return 1 + print(json.dumps(result, ensure_ascii=False, indent=2, sort_keys=True)) + return 0 + finally: + await store.close() + + +async def async_main(args: argparse.Namespace) -> int: + """Dispatch one CLI command.""" + if args.command == "show": + return await _show(args) + return await _run(args) + + +def main() -> int: + """Run CLI with redacted failure output.""" + try: + return asyncio.run(async_main(parse_args())) + except Exception as exc: # pylint: disable=broad-except + message = SecretRedactor().redact_text(str(exc)) + print(f"error: {type(exc).__name__}: {message}") + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/examples/skills_code_review_agent/scripts/init_db.py b/examples/skills_code_review_agent/scripts/init_db.py new file mode 100644 index 000000000..10dbed328 --- /dev/null +++ b/examples/skills_code_review_agent/scripts/init_db.py @@ -0,0 +1,37 @@ +#!/usr/bin/env python3 +"""Initialize the code-review example database.""" + +from __future__ import annotations + +import argparse +import asyncio +import os +from pathlib import Path +import sys + +EXAMPLE_ROOT = Path(__file__).resolve().parents[1] +if str(EXAMPLE_ROOT) not in sys.path: + sys.path.insert(0, str(EXAMPLE_ROOT)) + + +def parse_args() -> argparse.Namespace: + """Parse database initialization arguments.""" + from agent.constants import DB_URL_ENV + from agent.constants import DEFAULT_DB_URL + + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--db-url", default=os.getenv(DB_URL_ENV, DEFAULT_DB_URL)) + return parser.parse_args() + + +async def initialize(db_url: str) -> None: + """Create missing review tables and close the engine.""" + from agent.storage import create_sql_storage + + storage = create_sql_storage(db_url) + await storage.create_sql_engine() + await storage.close() + + +if __name__ == "__main__": + asyncio.run(initialize(parse_args().db_url)) diff --git a/examples/skills_code_review_agent/skills/code-review/SKILL.md b/examples/skills_code_review_agent/skills/code-review/SKILL.md new file mode 100644 index 000000000..5613d8a90 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/SKILL.md @@ -0,0 +1,32 @@ +--- +name: code-review +description: Review normalized Git changes for security, async, resource, test, secret, and database defects. +--- + +# Code Review + +Use this Skill only for structured code review. The caller stages a canonical +`review_input.json`; do not read unrelated workspace files. + +## Required flow + +1. Read `references/rules.md`. +2. Run the fixed scanner: + + ```text + python scripts/scan_rules.py --input work/inputs/review_input.json --output out/findings.jsonl + ``` + +3. Return `out/findings.jsonl` as the primary output. +4. Do not invoke a shell, network client, package manager, or another script. +5. Do not print environment variables or full input content. + +Each JSONL object must contain exactly: + +```text +severity, category, file, line, title, evidence, +recommendation, confidence, source +``` + +Only report changed lines or bounded hunk context. Evidence must be short and +must replace detected credential values with `[REDACTED]`. diff --git a/examples/skills_code_review_agent/skills/code-review/references/rules.md b/examples/skills_code_review_agent/skills/code-review/references/rules.md new file mode 100644 index 000000000..4fc92de88 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/references/rules.md @@ -0,0 +1,53 @@ +# Code review rules + +The scanner applies deterministic, explainable checks. A match is a candidate, +not proof; confidence reflects expected precision. + +## Security + +- Flag added dynamic execution such as `eval`, `exec`, `pickle.loads`, unsafe + YAML loading, or subprocess calls with `shell=True`. +- Flag SQL assembled from untrusted strings. +- Do not flag safe literal parsing or parameterized SQL. + +## Async errors + +- Flag a coroutine call assigned or invoked in `async def` without `await` + when the changed line clearly identifies an async API. +- Flag blocking `time.sleep`, synchronous HTTP, or blocking subprocess calls + added to an async function. +- Do not guess when surrounding context cannot establish async execution. + +## Resource leaks + +- Flag added `open`, process, lock, cursor, or connection acquisition without a + context manager or visible `try/finally` cleanup in the hunk. +- Do not flag `with`/`async with` ownership. + +## Missing tests + +- Flag production behavior changes when no test file appears in the same input. +- Emit one finding per changed production file, not per line. +- Documentation, fixture, generated, and configuration-only changes are exempt. + +## Secret leaks + +- Flag API keys, bearer tokens, private-key headers, URL credentials, and + password assignments. +- Evidence must mask the value before leaving the scanner. +- Placeholder, example, environment lookup, and already-redacted values are + exempt. + +## Database lifecycle + +- Flag transactions that return or raise without rollback/managed context. +- Flag connections or cursors acquired without close/context management. +- Flag commits inside exception handlers where rollback is expected. + +## Severity and confidence + +- `critical`: directly exploitable secret or command/code execution. +- `high`: likely security, transaction, or credential defect. +- `medium`: async/resource defect or important missing test. +- `low`: weak heuristic. +- Confidence below `0.70` is routed to warning and human review by the caller. diff --git a/examples/skills_code_review_agent/skills/code-review/scripts/scan_rules.py b/examples/skills_code_review_agent/skills/code-review/scripts/scan_rules.py new file mode 100644 index 000000000..fe0ac789d --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/scripts/scan_rules.py @@ -0,0 +1,442 @@ +#!/usr/bin/env python3 +"""Deterministic scanner for canonical code-review input.""" + +from __future__ import annotations + +import argparse +import json +import re +from dataclasses import dataclass +from pathlib import Path +from typing import Any +from typing import Iterable + +MAX_INPUT_BYTES = 2 * 1024 * 1024 +MAX_OUTPUT_LINES = 2_000 +MAX_EVIDENCE_LENGTH = 240 +MIN_SECRET_LENGTH = 8 +REDACTION_MARKER = "[REDACTED]" + +TEST_PATH_PATTERN = re.compile(r"(^|/)(tests?|specs?)(/|$)|(^|/)test_[^/]+\.py$") +EXEMPT_PATH_PATTERN = re.compile(r"\.(md|rst|txt|json|ya?ml|toml)$|(^|/)(fixtures?|generated)(/|$)") +ASYNC_CONTEXT_PATTERN = re.compile(r"\basync\s+def\b") +SAFE_RESOURCE_PATTERN = re.compile(r"\b(?:async\s+)?with\b") +SECRET_PATTERN = re.compile( + r"(?ix)" + r"[\"']?(?:api[_-]?key|access[_-]?token|secret|password)[\"']?" + r"\s*[:=]\s*[\"'](?P[^\"']{8,})[\"']" + r"|bearer\s+(?P[A-Za-z0-9._~+/=-]{8,})" + r"|(?Phttps?://[^/\s:@]+):(?P[^@\s/]+)@", ) +PRIVATE_KEY_PATTERN = re.compile(r"-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----") +TOKEN_PATTERN = re.compile( + r"\b(?:AKIA[0-9A-Z]{16}|AIza[0-9A-Za-z_-]{35}|gh[pousr]_[A-Za-z0-9]{30,}|" + r"sk-[A-Za-z0-9_-]{20,}|eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b", ) +BEHAVIOR_PATTERN = re.compile( + r"^\s*(?:async\s+def|def|class|if|for|while|return|raise|yield|await)\b" + r"|^\s*\w+(?:\.\w+)*\s*=.*\(" + r"|^\s*\w+(?:\.\w+)*\s*\(", ) +SECRET_NAME_PATTERN = re.compile(r"(?i)\b(?:api[_-]?key|access[_-]?token|secret|password)\b") +SECRET_ASSIGNMENT_PATTERN = re.compile( + r"(?i)[\"']?(?:api[_-]?key|access[_-]?token|secret|password)[\"']?" + r"\s*[:=]\s*(?P.+)$", ) +QUOTED_CHUNK_PATTERN = re.compile(r"[\"']([^\"']+)[\"']") +PLACEHOLDER_PATTERN = re.compile(r"(?i)example|placeholder|changeme|dummy|redacted") +SECRET_VALUE_GROUPS = ("value", "bearer", "url_secret") + + +@dataclass(frozen=True) +class Rule: + """One regex-backed changed-line rule.""" + + rule_id: str + pattern: re.Pattern[str] + finding: dict[str, Any] + flags: frozenset[str] = frozenset() + + +RULES = ( + Rule( + "security.dynamic-execution", + re.compile(r"\b(?:eval|exec|pickle\.loads)\s*\("), + { + "category": "security", + "severity": "high", + "title": "Dynamic execution on changed input", + "recommendation": "Replace dynamic execution with a typed parser or strict allowlist.", + "confidence": 0.93, + }, + ), + Rule( + "security.shell-true", + re.compile(r"\b(?:subprocess\.\w+|Popen)\s*\([^)]*\bshell\s*=\s*True"), + { + "category": "security", + "severity": "critical", + "title": "Shell execution enabled", + "recommendation": "Pass a fixed argv list and keep shell execution disabled.", + "confidence": 0.98, + }, + ), + Rule( + "security.unsafe-yaml", + re.compile(r"\byaml\.load\s*\((?![^)]*(?:SafeLoader|safe_load))"), + { + "category": "security", + "severity": "high", + "title": "Unsafe YAML deserialization", + "recommendation": "Use yaml.safe_load or SafeLoader.", + "confidence": 0.94, + }, + ), + Rule( + "security.sql-interpolation", + re.compile(r"\.execute\s*\(\s*(?:f[\"']|[^)]*\.format\s*\(|[^)]*%\s*[\w(])"), + { + "category": "security", + "severity": "high", + "title": "SQL statement uses string interpolation", + "recommendation": "Use a parameterized query and bind values separately.", + "confidence": 0.91, + }, + ), + Rule( + "async.blocking-sleep", + re.compile(r"\btime\.sleep\s*\("), + { + "category": "async_error", + "severity": "medium", + "title": "Blocking sleep in async code", + "recommendation": "Use await asyncio.sleep instead.", + "confidence": 0.95, + }, + frozenset({"async"}), + ), + Rule( + "async.unawaited-call", + re.compile(r"^(?!\s*(?:await|return\s+await)\b).*\b(?:fetch_async|request_async|execute_async)\s*\("), + { + "category": "async_error", + "severity": "medium", + "title": "Async call is not awaited", + "recommendation": "Await the coroutine or schedule and track it explicitly.", + "confidence": 0.88, + }, + frozenset({"async"}), + ), + Rule( + "async.sync-http", + re.compile(r"\b(?:requests|urllib\.request|httpx)\.(?:get|post|put|delete|request)\s*\("), + { + "category": "async_error", + "severity": "medium", + "title": "Synchronous HTTP call in async code", + "recommendation": "Use an async HTTP client and await the request.", + "confidence": 0.88, + }, + frozenset({"async"}), + ), + Rule( + "async.blocking-process", + re.compile(r"\bsubprocess\.(?:run|call|check_call|check_output)\s*\("), + { + "category": "async_error", + "severity": "medium", + "title": "Blocking subprocess call in async code", + "recommendation": "Use asyncio subprocess APIs and await completion.", + "confidence": 0.86, + }, + frozenset({"async"}), + ), + Rule( + "resource.unmanaged-file", + re.compile(r"=\s*open\s*\("), + { + "category": "resource_leak", + "severity": "medium", + "title": "File opened without managed lifetime", + "recommendation": "Use a with statement or close the file in finally.", + "confidence": 0.82, + }, + frozenset({"unmanaged"}), + ), + Rule( + "resource.unmanaged-process", + re.compile(r"=\s*(?:subprocess\.)?Popen\s*\("), + { + "category": "resource_leak", + "severity": "medium", + "title": "Process lifetime is unmanaged", + "recommendation": "Use a context manager and enforce wait/terminate cleanup.", + "confidence": 0.80, + }, + frozenset({"unmanaged"}), + ), + Rule( + "resource.unmanaged-lock", + re.compile(r"(?:await\s+)?\w+\.acquire\s*\("), + { + "category": "resource_leak", + "severity": "medium", + "title": "Lock acquisition lacks managed release", + "recommendation": "Use a context manager or release the lock in finally.", + "confidence": 0.78, + }, + frozenset({"unmanaged"}), + ), + Rule( + "db.unmanaged-connection", + re.compile(r"=\s*(?:engine|db|pool)\.(?:connect|acquire)\s*\("), + { + "category": "db_lifecycle", + "severity": "high", + "title": "Database connection lifetime is unmanaged", + "recommendation": "Use a managed transaction/context and guarantee rollback or close.", + "confidence": 0.90, + }, + frozenset({"unmanaged"}), + ), + Rule( + "db.unmanaged-cursor", + re.compile(r"=\s*\w+\.cursor\s*\("), + { + "category": "db_lifecycle", + "severity": "medium", + "title": "Database cursor lifetime is unmanaged", + "recommendation": "Use a context manager or close the cursor in finally.", + "confidence": 0.86, + }, + frozenset({"unmanaged"}), + ), + Rule( + "db.unmanaged-transaction", + re.compile(r"=\s*(?:connection|session|db)\.begin\s*\("), + { + "category": "db_lifecycle", + "severity": "high", + "title": "Database transaction lifetime is unmanaged", + "recommendation": "Use a transaction context and rollback on every failure path.", + "confidence": 0.88, + }, + frozenset({"unmanaged"}), + ), + Rule( + "db.commit-in-except", + re.compile(r"\.(?:commit)\s*\("), + { + "category": "db_lifecycle", + "severity": "high", + "title": "Transaction committed from exception path", + "recommendation": "Rollback on failure and commit only after successful work.", + "confidence": 0.84, + }, + frozenset({"except"}), + ), +) + + +def _load_input(path: Path) -> dict[str, Any]: + data = path.read_bytes() + if len(data) > MAX_INPUT_BYTES: + raise ValueError("review input exceeds configured limit") + if b"\x00" in data: + raise ValueError("review input contains NUL bytes") + payload = json.loads(data) + if not isinstance(payload, dict) or not isinstance(payload.get("files"), list): + raise ValueError("review input must contain a files array") + return payload + + +def _mask_secret(text: str) -> str: + masked = PRIVATE_KEY_PATTERN.sub(REDACTION_MARKER, text) + masked = TOKEN_PATTERN.sub(REDACTION_MARKER, masked) + + def replace(match: re.Match[str]) -> str: + value = match.group(0) + for secret in match.groupdict().values(): + if secret and len(secret) >= MIN_SECRET_LENGTH: + value = value.replace(secret, REDACTION_MARKER) + return value + + return SECRET_PATTERN.sub(replace, masked)[:MAX_EVIDENCE_LENGTH] + + +def _contains_secret(content: str, context: str) -> bool: + secret_match = SECRET_PATTERN.search(content) + if secret_match: + values = [secret_match.group(name) for name in SECRET_VALUE_GROUPS if secret_match.group(name)] + return any(not PLACEHOLDER_PATTERN.search(item) for item in values) + if TOKEN_PATTERN.search(content): + return True + if PRIVATE_KEY_PATTERN.search(content): + return True + if not SECRET_NAME_PATTERN.search(content): + return False + assignment = SECRET_ASSIGNMENT_PATTERN.search(content) + if not assignment: + return False + chunks = QUOTED_CHUNK_PATTERN.findall(assignment.group("value")) + combined = "".join(chunks) + return len(combined) >= MIN_SECRET_LENGTH and not PLACEHOLDER_PATTERN.search(combined) + + +def _finding(rule: Rule, file_path: str, line: int | None, evidence: str) -> dict[str, Any]: + return { + "severity": rule.finding["severity"], + "category": rule.finding["category"], + "file": file_path, + "line": line, + "title": rule.finding["title"], + "evidence": _mask_secret(evidence), + "recommendation": rule.finding["recommendation"], + "confidence": rule.finding["confidence"], + "source": f"skill:code-review/{rule.rule_id}", + } + + +def _iter_added_lines(review_file: dict[str, Any]) -> Iterable[tuple[int | None, str, str]]: + for hunk in review_file.get("hunks", []): + context = "\n".join( + str(line.get("content", "")) for line in hunk.get("lines", []) if line.get("kind") != "deleted") + for line in hunk.get("lines", []): + if line.get("kind") == "added": + yield line.get("new_line"), str(line.get("content", "")), context + + +def _rule_matches(rule: Rule, content: str, context: str) -> bool: + if not rule.pattern.search(content): + return False + if "async" in rule.flags and not ASYNC_CONTEXT_PATTERN.search(context): + return False + if "except" in rule.flags and "except" not in context: + return False + if "unmanaged" in rule.flags and _has_visible_cleanup(content, context): + return False + return True + + +def _has_visible_cleanup(content: str, context: str) -> bool: + if SAFE_RESOURCE_PATTERN.search(content): + return True + assignment = re.match(r"\s*(\w+)\s*=", content) + target = assignment.group(1) if assignment else None + if target: + cleanup = re.compile(rf"\b{re.escape(target)}\.(?:close|release|wait|terminate)\s*\(") + return bool(cleanup.search(context)) + lock = re.search(r"\b(\w+)\.acquire\s*\(", content) + if lock: + return bool(re.search(rf"\b{re.escape(lock.group(1))}\.release\s*\(", context)) + return False + + +def _scan_line_rules(review_file: dict[str, Any]) -> list[dict[str, Any]]: + path = str(review_file.get("new_path") or review_file.get("old_path") or "") + findings: list[dict[str, Any]] = [] + for line_number, content, context in _iter_added_lines(review_file): + for rule in RULES: + if _rule_matches(rule, content, context): + findings.append(_finding(rule, path, line_number, content)) + if _contains_secret(content, context): + secret_rule = Rule( + "secret.literal", + SECRET_PATTERN, + { + "category": "secret_leak", + "severity": "critical", + "title": "Credential material added", + "recommendation": "Remove the secret, rotate it, and load credentials from a secret store.", + "confidence": 0.99, + }, + ) + findings.append(_finding(secret_rule, path, line_number, content)) + return findings + + +def _has_test_changes(files: list[dict[str, Any]]) -> bool: + return any(TEST_PATH_PATTERN.search(str(item.get("new_path") or item.get("old_path") or "")) for item in files) + + +def _first_added_line(review_file: dict[str, Any]) -> int | None: + for line_number, content, _ in _iter_added_lines(review_file): + stripped = content.strip() + if stripped and not stripped.startswith(("#", "import ", "from ")): + if BEHAVIOR_PATTERN.search(content): + return line_number + return None + + +def _missing_test_findings(files: list[dict[str, Any]]) -> list[dict[str, Any]]: + if _has_test_changes(files): + return [] + rule = Rule( + "tests.missing", + re.compile(""), + { + "category": "missing_test", + "severity": "medium", + "title": "Production change has no test update", + "recommendation": "Add a focused test covering the changed behavior and failure path.", + "confidence": 0.76, + }, + ) + findings = [] + for review_file in files: + path = str(review_file.get("new_path") or review_file.get("old_path") or "") + if not path or TEST_PATH_PATTERN.search(path) or EXEMPT_PATH_PATTERN.search(path): + continue + line = _first_added_line(review_file) + if line is not None: + findings.append(_finding(rule, path, line, path)) + return findings + + +def _deduplicate(findings: list[dict[str, Any]]) -> list[dict[str, Any]]: + selected: dict[tuple[str, int | None, str], dict[str, Any]] = {} + for finding in findings: + key = (finding["file"], finding["line"], finding["category"]) + previous = selected.get(key) + if previous is None or finding["confidence"] > previous["confidence"]: + selected[key] = finding + return sorted( + selected.values(), + key=lambda item: (item["file"], item["line"] or 0, item["category"]), + ) + + +def scan(payload: dict[str, Any]) -> list[dict[str, Any]]: + """Return deterministic findings for one canonical review input.""" + files = payload["files"] + findings = [] + for review_file in files: + if not review_file.get("is_binary"): + findings.extend(_scan_line_rules(review_file)) + findings.extend(_missing_test_findings(files)) + return _deduplicate(findings)[:MAX_OUTPUT_LINES] + + +def _write_findings(path: Path, findings: list[dict[str, Any]]) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + with path.open("w", encoding="utf-8", newline="\n") as output: + for finding in findings: + output.write(json.dumps(finding, ensure_ascii=False, sort_keys=True)) + output.write("\n") + + +def parse_args() -> argparse.Namespace: + """Parse fixed scanner arguments.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--input", required=True, type=Path) + parser.add_argument("--output", required=True, type=Path) + return parser.parse_args() + + +def main() -> int: + """Run scanner and write JSONL output.""" + args = parse_args() + findings = scan(_load_input(args.input)) + _write_findings(args.output, findings) + print(f"findings_written={len(findings)}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/examples/skills_code_review_agent/test_acceptance.py b/tests/examples/skills_code_review_agent/test_acceptance.py new file mode 100644 index 000000000..96587dbc5 --- /dev/null +++ b/tests/examples/skills_code_review_agent/test_acceptance.py @@ -0,0 +1,218 @@ +"""Issue #92 acceptance tests over public fixtures and full fake Agent flow.""" + +from __future__ import annotations + +import ast +import json +import os +from pathlib import Path +import runpy +import time + +from mccabe import get_code_complexity +import pytest + +from examples.skills_code_review_agent.agent.input_parser import load_diff_file +from examples.skills_code_review_agent.agent.models import SandboxStatus +from examples.skills_code_review_agent.agent.models import SandboxRun +from examples.skills_code_review_agent.agent.models import TaskStatus +from examples.skills_code_review_agent.agent.pipeline import FAKE_MODEL_NAME +from examples.skills_code_review_agent.agent.pipeline import FakeReviewModel +from examples.skills_code_review_agent.agent.pipeline import PipelineDependencies +from examples.skills_code_review_agent.agent.pipeline import ReviewPipeline +from examples.skills_code_review_agent.agent.policy import SecretRedactor +from examples.skills_code_review_agent.agent.sandbox import create_runtime +from examples.skills_code_review_agent.agent.sandbox import SandboxExecution +from examples.skills_code_review_agent.agent.sandbox import SandboxExecutor +from examples.skills_code_review_agent.agent.storage import ReviewStore + +EXAMPLE_ROOT = Path("examples/skills_code_review_agent") +FIXTURE_ROOT = EXAMPLE_ROOT / "fixtures" +SKILL_ROOT = EXAMPLE_ROOT / "skills" +SCANNER_PATH = SKILL_ROOT / "code-review/scripts/scan_rules.py" +EXPECTED_FIXTURE_COUNT = 8 +MIN_DETECTION_RATE = 0.80 +MAX_FALSE_POSITIVE_RATE = 0.15 +MAX_FAKE_SECONDS = 120 +MAX_FILE_LINES = 1_000 +MAX_FUNCTION_LINES = 80 +MAX_FUNCTION_STATEMENTS = 60 +MAX_FUNCTION_PARAMETERS = 4 +MAX_COMPLEXITY = 15 +SUCCESS_FIXTURES = ( + "async_resource", + "clean", + "db_lifecycle", + "duplicate", + "missing_tests", + "secrets", + "security", +) + + +def _load_scanner(): + return runpy.run_path(str(SCANNER_PATH))["scan"] + + +def _key(item: dict) -> tuple[str, str, int | None]: + return item["category"], item["file"], item["line"] + + +def test_public_fixtures_meet_detection_and_false_positive_targets() -> None: + scanner = _load_scanner() + expected_files = sorted(FIXTURE_ROOT.glob("*.expected.json")) + assert len(expected_files) == EXPECTED_FIXTURE_COUNT + expected_total = detected = false_positives = actual_total = 0 + for expected_path in expected_files: + expected = json.loads(expected_path.read_text(encoding="utf-8")) + expected_keys = {_key(item) for item in expected["expected_findings"]} + if expected["expected_sandbox_status"] == SandboxStatus.TIMED_OUT.value: + assert not expected_keys + continue + diff_name = expected_path.name.replace(".expected.json", ".diff") + review_input = load_diff_file(expected_path.with_name(diff_name)) + actual_keys = {_key(item) for item in scanner(review_input.model_dump(mode="json"))} + expected_total += len(expected_keys) + actual_total += len(actual_keys) + detected += len(expected_keys & actual_keys) + false_positives += len(actual_keys - expected_keys) + assert len(actual_keys - expected_keys) <= expected["max_false_positives"] + assert detected / expected_total >= MIN_DETECTION_RATE + denominator = max(actual_total, 1) + assert false_positives / denominator <= MAX_FALSE_POSITIVE_RATE + + +@pytest.mark.asyncio +@pytest.mark.parametrize("fixture_name", SUCCESS_FIXTURES) +async def test_fake_agent_runs_skill_filter_sandbox_db_and_report( + fixture_name, + tmp_path, + monkeypatch, +) -> None: + monkeypatch.setenv("TRPC_CODE_REVIEW_ALLOW_UNSAFE_LOCAL", "1") + store = ReviewStore( + f"sqlite:///{(tmp_path / 'review.db').as_posix()}", + SecretRedactor(), + ) + await store.initialize() + started = time.monotonic() + try: + dependencies = PipelineDependencies( + store=store, + runtime=create_runtime("local", tmp_path / "work"), + skill_root=SKILL_ROOT, + output_dir=tmp_path / "reports", + model=FakeReviewModel(model_name=FAKE_MODEL_NAME), + ) + report, json_path, markdown_path = await ReviewPipeline(dependencies).run( + load_diff_file(FIXTURE_ROOT / f"{fixture_name}.diff"), ) + persisted = await store.get_report(report.task_id) + finally: + await store.close() + assert report.status == TaskStatus.COMPLETE, report.failures + assert report.metrics.tool_calls == 2 + expected = json.loads((FIXTURE_ROOT / f"{fixture_name}.expected.json").read_text(encoding="utf-8"), ) + actual_keys = {_key(item.model_dump(mode="json")) for item in report.findings} + expected_keys = {_key(item) for item in expected["expected_findings"]} + assert len(report.findings) == len(actual_keys) + assert actual_keys == expected_keys + if fixture_name == "duplicate": + security = next(item for item in report.findings if item.category.value == "security") + assert security.source == "skill:code-review/security.shell-true" + assert report.filter_decisions[0].action.value == "allow" + assert report.sandbox_runs[0].status == SandboxStatus.SUCCEEDED + assert persisted["report"]["task_id"] == report.task_id + assert json_path.is_file() and markdown_path.is_file() + assert time.monotonic() - started < MAX_FAKE_SECONDS + assert not list((tmp_path / "work").glob("ws_*")) + + +@pytest.mark.asyncio +async def test_sandbox_failure_fixture_runs_full_recovery_path( + tmp_path, + monkeypatch, +) -> None: + monkeypatch.setenv("TRPC_CODE_REVIEW_ALLOW_UNSAFE_LOCAL", "1") + + async def timeout(executor, plan, input_bytes): + del executor, plan, input_bytes + return SandboxExecution( + run=SandboxRun( + status=SandboxStatus.TIMED_OUT, + timed_out=True, + duration_ms=1, + error_type="TimeoutError", + ), + output="", + ) + + monkeypatch.setattr(SandboxExecutor, "execute", timeout) + store = ReviewStore( + f"sqlite:///{(tmp_path / 'failure.db').as_posix()}", + SecretRedactor(), + ) + await store.initialize() + try: + dependencies = PipelineDependencies( + store=store, + runtime=create_runtime("local", tmp_path / "work"), + skill_root=SKILL_ROOT, + output_dir=tmp_path / "reports", + model=FakeReviewModel(model_name=FAKE_MODEL_NAME), + ) + report, _, _ = await ReviewPipeline(dependencies).run(load_diff_file(FIXTURE_ROOT / "sandbox_failure.diff"), ) + finally: + await store.close() + assert report.status == TaskStatus.FAILED + assert report.sandbox_runs[0].status == SandboxStatus.TIMED_OUT + assert report.metrics.exceptions_by_type == {"TimeoutError": 1} + assert not report.findings + + +@pytest.mark.asyncio +@pytest.mark.skipif( + os.getenv("TRPC_CODE_REVIEW_CONTAINER_TEST") != "1", + reason="set TRPC_CODE_REVIEW_CONTAINER_TEST=1 to run Docker integration", +) +async def test_container_pipeline_when_enabled(tmp_path) -> None: + store = ReviewStore( + f"sqlite:///{(tmp_path / 'container.db').as_posix()}", + SecretRedactor(), + ) + await store.initialize() + try: + dependencies = PipelineDependencies( + store=store, + runtime=create_runtime("container"), + skill_root=SKILL_ROOT, + output_dir=tmp_path / "reports", + model=FakeReviewModel(model_name=FAKE_MODEL_NAME), + ) + report, _, _ = await ReviewPipeline(dependencies).run(load_diff_file(FIXTURE_ROOT / "security.diff"), ) + finally: + await store.close() + assert report.status == TaskStatus.COMPLETE + + +def test_python_source_meets_static_limits() -> None: + example_sources = [path for path in EXAMPLE_ROOT.rglob("*.py") if SKILL_ROOT not in path.parents] + sources = [*example_sources, *Path(__file__).parent.glob("*.py")] + for path in sources: + text = path.read_text(encoding="utf-8") + assert len(text.splitlines()) <= MAX_FILE_LINES, path + tree = ast.parse(text) + assert get_code_complexity(text, MAX_COMPLEXITY, str(path)) == 0 + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + assert node.end_lineno - node.lineno + 1 <= MAX_FUNCTION_LINES, ( + path, + node.name, + ) + statements = sum(isinstance(item, ast.stmt) for item in ast.walk(node)) + assert statements <= MAX_FUNCTION_STATEMENTS, (path, node.name) + arguments = [ + *node.args.posonlyargs, + *node.args.args, + *node.args.kwonlyargs, + ] + assert len(arguments) <= MAX_FUNCTION_PARAMETERS, (path, node.name) diff --git a/tests/examples/skills_code_review_agent/test_cli.py b/tests/examples/skills_code_review_agent/test_cli.py new file mode 100644 index 000000000..075d4b12c --- /dev/null +++ b/tests/examples/skills_code_review_agent/test_cli.py @@ -0,0 +1,85 @@ +"""CLI smoke tests for durable run and query commands.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +import subprocess +import sys + +CLI_PATH = Path("examples/skills_code_review_agent/run_agent.py") +CLI_TIMEOUT_SECONDS = 30 + + +def _run_cli( + arguments: list[str], + environment_updates: dict[str, str] | None = None, +) -> subprocess.CompletedProcess[str]: + environment = os.environ.copy() + environment["PYTHONUTF8"] = "1" + environment.update(environment_updates or {}) + return subprocess.run( + [sys.executable, str(CLI_PATH), *arguments], + check=False, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=CLI_TIMEOUT_SECONDS, + env=environment, + ) + + +def test_dry_run_can_be_queried_by_task_id(tmp_path) -> None: + database_url = f"sqlite:///{(tmp_path / 'cli.db').as_posix()}" + output_dir = tmp_path / "reports" + result = _run_cli( + [ + "run", + "--fixture", + "clean", + "--dry-run", + "--db-url", + database_url, + "--output-dir", + str(output_dir), + ], + {"DOCKER_HOST": "tcp://127.0.0.1:1"}, + ) + assert result.returncode == 0, result.stdout + result.stderr + task_line = next(line for line in result.stdout.splitlines() if line.startswith("task_id=")) + task_id = task_line.partition("=")[2] + + query = _run_cli([ + "show", + "--task-id", + task_id, + "--db-url", + database_url, + ]) + assert query.returncode == 0, query.stdout + query.stderr + payload = json.loads(query.stdout) + assert payload["task_id"] == task_id + assert payload["status"] == "complete" + assert payload["report"]["sandbox_runs"] == [] + + +def test_database_initializer_entrypoint_runs(tmp_path) -> None: + database_url = f"sqlite:///{(tmp_path / 'initialized.db').as_posix()}" + result = subprocess.run( + [ + sys.executable, + "examples/skills_code_review_agent/scripts/init_db.py", + "--db-url", + database_url, + ], + check=False, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=CLI_TIMEOUT_SECONDS, + ) + assert result.returncode == 0, result.stdout + result.stderr + assert (tmp_path / "initialized.db").is_file() diff --git a/tests/examples/skills_code_review_agent/test_input_parser.py b/tests/examples/skills_code_review_agent/test_input_parser.py new file mode 100644 index 000000000..30c9f5a56 --- /dev/null +++ b/tests/examples/skills_code_review_agent/test_input_parser.py @@ -0,0 +1,178 @@ +"""Tests for canonical code-review input and static Skill rules.""" + +from __future__ import annotations + +import json +import runpy +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from examples.skills_code_review_agent.agent.input_parser import GitDiffOptions +from examples.skills_code_review_agent.agent.input_parser import InputValidationError +from examples.skills_code_review_agent.agent.input_parser import load_diff_file +from examples.skills_code_review_agent.agent.input_parser import load_file_list +from examples.skills_code_review_agent.agent.input_parser import load_repo_diff +from examples.skills_code_review_agent.agent.input_parser import parse_diff_text +from examples.skills_code_review_agent.agent.models import Finding + +EXAMPLE_ROOT = Path("examples/skills_code_review_agent") +FIXTURE_ROOT = EXAMPLE_ROOT / "fixtures" +SCANNER_PATH = EXAMPLE_ROOT / "skills/code-review/scripts/scan_rules.py" +SCAN = runpy.run_path(str(SCANNER_PATH))["scan"] + + +@pytest.mark.parametrize("diff_path", sorted(FIXTURE_ROOT.glob("*.diff"))) +def test_all_public_fixtures_parse(diff_path): + review_input = load_diff_file(diff_path) + + assert review_input.files + assert len(review_input.digest) == 64 + + +def test_plain_unified_diff_parses_without_git_header(): + review_input = parse_diff_text("--- a/app.py\n" + "+++ b/app.py\n" + "@@ -1,1 +1,1 @@\n" + "-old\n" + "+new\n", ) + + assert review_input.files[0].new_path == "app.py" + assert review_input.files[0].hunks[0].lines[-1].new_line == 1 + + +def test_hunk_count_mismatch_is_rejected(): + malformed = ("diff --git a/app.py b/app.py\n" + "--- a/app.py\n" + "+++ b/app.py\n" + "@@ -1,2 +1,1 @@\n" + "-old\n" + "+new\n") + + with pytest.raises(InputValidationError, match="hunk count mismatch"): + parse_diff_text(malformed) + + +@pytest.mark.parametrize( + "path", + [ + "C:/outside.py", + r"\\server\share\outside.py", + "../outside.py", + "/outside.py", + "safe/\x00bad.py", + ], +) +def test_finding_rejects_unsafe_paths(path): + with pytest.raises(ValidationError): + Finding( + severity="high", + category="security", + file=path, + line=1, + title="unsafe", + evidence="evidence", + recommendation="fix", + confidence=0.9, + source="test", + ) + + +def test_file_list_digest_includes_file_content(tmp_path): + source = tmp_path / "app.py" + source.write_text("value = 1\n", encoding="utf-8") + file_list = tmp_path / "files.txt" + file_list.write_text("app.py\n", encoding="utf-8") + first = load_file_list(file_list) + + source.write_text("value = 2\n", encoding="utf-8") + second = load_file_list(file_list) + + assert first.digest != second.digest + + +def test_file_list_rejects_empty_non_utf8_and_symlink(tmp_path): + empty = tmp_path / "empty.txt" + empty.write_text("", encoding="utf-8") + with pytest.raises(InputValidationError, match="must not be empty"): + load_file_list(empty) + + invalid = tmp_path / "invalid.txt" + invalid.write_bytes(b"\xff") + with pytest.raises(InputValidationError, match="UTF-8"): + load_file_list(invalid) + + +def test_repo_revision_rejects_option_injection(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + "examples.skills_code_review_agent.agent.input_parser._run_git_diff", + lambda root, arguments: "", + ) + + options = GitDiffOptions(base="main", head="--output=outside") + with pytest.raises(InputValidationError, match="unsafe Git revision"): + load_repo_diff(tmp_path, options) + + +def test_binary_diff_becomes_warning(): + review_input = parse_diff_text( + "diff --git a/image.png b/image.png\n" + "Binary files a/image.png and b/image.png differ\n", ) + + assert review_input.files[0].is_binary is True + assert review_input.warnings == ["binary file skipped: image.png"] + + +@pytest.mark.parametrize( + ("fixture", "category", "line"), + [ + ("security", "security", 4), + ("async_resource", "async_error", 2), + ("async_resource", "resource_leak", 3), + ("db_lifecycle", "db_lifecycle", 2), + ("missing_tests", "missing_test", 2), + ("secrets", "secret_leak", 2), + ], +) +def test_scanner_detects_required_rule_categories(fixture, category, line): + review_input = load_diff_file(FIXTURE_ROOT / f"{fixture}.diff") + findings = SCAN(review_input.model_dump(mode="json")) + + assert any(item["category"] == category and item["line"] == line for item in findings) + + +def test_scanner_clean_fixture_has_no_false_positive(): + review_input = load_diff_file(FIXTURE_ROOT / "clean.diff") + expected = json.loads((FIXTURE_ROOT / "clean.expected.json").read_text()) + + assert expected["expected_findings"] == [] + assert SCAN(review_input.model_dump(mode="json")) == [] + + +def test_scanner_redacts_quoted_secret_key_with_example_context(): + secret = "CorrectHorseBatteryStaple" + review_input = parse_diff_text( + "diff --git a/config.py b/config.py\n" + "--- a/config.py\n" + "+++ b/config.py\n" + "@@ -0,0 +1 @@\n" + f'+settings = {{"password": "{secret}"}} # example config\n', ) + + findings = SCAN(review_input.model_dump(mode="json")) + + assert findings[0]["category"] == "secret_leak" + assert secret not in findings[0]["evidence"] + assert "[REDACTED]" in findings[0]["evidence"] + + +def test_scanner_ignores_placeholder_secret_value(): + review_input = parse_diff_text( + "diff --git a/config.py b/config.py\n" + "--- a/config.py\n" + "+++ b/config.py\n" + "@@ -0,0 +1 @@\n" + '+url = "https://user:changeme@example.invalid/path"\n', ) + + assert SCAN(review_input.model_dump(mode="json")) == [] diff --git a/tests/examples/skills_code_review_agent/test_policy_sandbox.py b/tests/examples/skills_code_review_agent/test_policy_sandbox.py new file mode 100644 index 000000000..00082fc6e --- /dev/null +++ b/tests/examples/skills_code_review_agent/test_policy_sandbox.py @@ -0,0 +1,397 @@ +"""Tests for policy, redaction, and sandbox boundaries.""" + +from __future__ import annotations + +import asyncio +import hashlib +from pathlib import Path +import stat + +import pytest +from pydantic import ValidationError +from trpc_agent_sdk.code_executors import CodeFile +from trpc_agent_sdk.code_executors import WorkspaceInfo +from trpc_agent_sdk.code_executors import WorkspaceRunResult + +from examples.skills_code_review_agent.agent.models import DecisionAction +from examples.skills_code_review_agent.agent.models import FilterDecision +from examples.skills_code_review_agent.agent.models import SandboxStatus +from examples.skills_code_review_agent.agent.policy import REDACTION_MARKER +from examples.skills_code_review_agent.agent.policy import ReviewPolicyFilter +from examples.skills_code_review_agent.agent.policy import SecretRedactor +from examples.skills_code_review_agent.agent.policy import build_execution_plan +from examples.skills_code_review_agent.agent.policy import calculate_plan_digest +from examples.skills_code_review_agent.agent.policy import run_guarded +from examples.skills_code_review_agent.agent import pipeline +from examples.skills_code_review_agent.agent.sandbox import RuntimeHandle +from examples.skills_code_review_agent.agent.sandbox import SandboxExecutor + +SKILL_DIR = Path("examples/skills_code_review_agent/skills/code-review") +INPUT_BYTES = b'{"files":[]}' + +SECRET_CASES = [ + 'api_key="abcdefghijklmnop"', + "password='correct-horse-battery'", + 'access_token="token-value-12345"', + 'client_secret="client-value-12345"', + "Authorization: Bearer abcdefghijklmnop", + "https://user:password123@example.invalid/path", + "AK" + "IAABCDEFGHIJKLMNOP", + "AI" + "zaABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghi", + "gh" + "p_ABCDEFGHIJKLMNOPQRSTUVWXYZ1234", + "gh" + "o_ABCDEFGHIJKLMNOPQRSTUVWXYZ1234", + "gh" + "u_ABCDEFGHIJKLMNOPQRSTUVWXYZ1234", + "s" + "k-abcdefghijklmnopqrstuvwxyz123456", + "ey" + "Jabcdefghijk.eyJmnopqrstuv.abcdefghijklmnop", + "-----BEGIN PRIVATE KEY-----\nsecret-body\n-----END PRIVATE KEY-----", + 'api_key = ("abcdefgh" + "ijklmnop")', + '密码="这是一个很长的测试密码"', + '密钥="unicode-secret-value"', + '令牌="unicode-token-value"', + 'password="c2VjcmV0LXZhbHVlLTEyMzQ1"', + "https://user:p%40ssword@example.invalid", + '"password": "CorrectHorseBatteryStaple"', +] + + +class FakeManager: + + def __init__(self, delay: float = 0.0, cleanup_error: bool = False): + self.delay = delay + self.cleanup_error = cleanup_error + self.cleanup_calls = 0 + + async def create_workspace(self, exec_id): + if self.delay: + await asyncio.sleep(self.delay) + return WorkspaceInfo(id=exec_id, path="/fake/workspace") + + async def cleanup(self, exec_id): + del exec_id + self.cleanup_calls += 1 + if self.cleanup_error: + raise RuntimeError("cleanup failed") + + +class FakeFS: + + def __init__(self, output: str = ""): + self.output = output + self.put_paths = [] + + async def put_files(self, workspace, files): + del workspace + self.put_paths = [item.path for item in files] + + async def collect(self, workspace, patterns): + del workspace, patterns + size = len(self.output.encode("utf-8")) + return [ + CodeFile( + name="out/findings.jsonl", + content=self.output, + mime_type="application/json", + size_bytes=size, + ), + ] + + +class FakeRunner: + + def __init__(self, delay: float = 0.0, stdout: str = ""): + self.delay = delay + self.stdout = stdout + self.calls = 0 + + async def run_program(self, workspace, spec): + del workspace, spec + self.calls += 1 + if self.delay: + await asyncio.sleep(self.delay) + return WorkspaceRunResult(stdout=self.stdout, exit_code=0) + + +class FakeRuntime: + + def __init__(self, manager=None, filesystem=None, runner=None): + self._manager = manager or FakeManager() + self._filesystem = filesystem or FakeFS() + self._runner = runner or FakeRunner() + + def manager(self): + return self._manager + + def fs(self): + return self._filesystem + + def runner(self): + return self._runner + + +def _executor(runtime=None): + fake = runtime or FakeRuntime() + handle = RuntimeHandle(runtime=fake, kind="container", network_disabled=True) + return SandboxExecutor(handle, SecretRedactor(), SKILL_DIR), fake + + +def _plan(executor, input_bytes=INPUT_BYTES): + return build_execution_plan( + "container", + hashlib.sha256(input_bytes).hexdigest(), + executor.skill_digest, + ) + + +def test_secret_redaction_recall_is_at_least_95_percent(): + redactor = SecretRedactor() + detected = sum(REDACTION_MARKER in redactor.redact_text(case) for case in SECRET_CASES) + + assert detected / len(SECRET_CASES) >= 0.95 + + +def test_structured_secret_values_are_redacted(): + redactor = SecretRedactor() + payload = { + "api_key": "short-but-sensitive", + "nested": [{ + "password": "another-sensitive-value" + }], + } + + assert redactor.redact_value(payload) == { + "api_key": REDACTION_MARKER, + "nested": [{ + "password": REDACTION_MARKER + }], + } + + +def test_execution_plan_is_frozen(): + executor, _ = _executor() + plan = _plan(executor) + + with pytest.raises(ValidationError): + plan.argv = ("python", "other.py") + + +def test_workspace_is_writable_before_links_are_removed(tmp_path, monkeypatch): + skill_dir = tmp_path / "skills" / "code-review" + skill_dir.mkdir(parents=True) + skill_dir.chmod(skill_dir.stat().st_mode & ~stat.S_IWRITE) + removed = [] + + monkeypatch.setattr( + pipeline, + "_is_workspace_link", + lambda path: path.name in pipeline.SKILL_WORKSPACE_LINK_NAMES, + ) + + def remove_link(path): + assert skill_dir.stat().st_mode & stat.S_IWRITE + removed.append(Path(path).name) + + monkeypatch.setattr(pipeline, "_remove_workspace_link", remove_link) + + pipeline._make_workspace_removable(tmp_path) + + assert removed == [] + + for name in pipeline.SKILL_WORKSPACE_LINK_NAMES: + (skill_dir / name).mkdir() + pipeline._make_workspace_removable(tmp_path) + + assert removed == list(pipeline.SKILL_WORKSPACE_LINK_NAMES) + + +def test_filter_denies_posix_absolute_path_with_valid_digest(): + executor, _ = _executor() + plan = _plan(executor).model_copy(update={ + "input_path": "/outside", + "digest": "pending", + }, ) + plan = plan.model_copy(update={"digest": calculate_plan_digest(plan)}) + + decisions = ReviewPolicyFilter(lambda items: _noop()).evaluate(plan) + + path_decision = next(item for item in decisions if item.rule_id == "path.workspace") + assert path_decision.action == DecisionAction.DENY + + +@pytest.mark.asyncio +async def test_denied_plan_never_calls_handler(): + executor, _ = _executor() + plan = _plan(executor).model_copy(update={"argv": ("python", "other.py")}, ) + decisions = [] + handler_calls = 0 + + async def audit(items): + decisions.extend(items) + + async def handler(approved): + nonlocal handler_calls + del approved + handler_calls += 1 + + result, _ = await run_guarded(plan, ReviewPolicyFilter(audit), handler) + + assert result is None + assert handler_calls == 0 + assert any(item.action == DecisionAction.DENY for item in decisions) + + +@pytest.mark.asyncio +async def test_needs_human_review_never_calls_handler(monkeypatch): + executor, _ = _executor() + plan = _plan(executor) + handler_calls = 0 + decision = FilterDecision( + action=DecisionAction.NEEDS_HUMAN_REVIEW, + rule_id="test.human", + reason="manual approval required", + plan_digest=plan.digest, + created_at="2026-01-01T00:00:00Z", + ) + policy = ReviewPolicyFilter(lambda items: _noop()) + monkeypatch.setattr(policy, "evaluate", lambda value: [decision]) + + async def handler(approved): + nonlocal handler_calls + del approved + handler_calls += 1 + + result, _ = await run_guarded(plan, policy, handler) + + assert result is None + assert handler_calls == 0 + + +async def _noop(): + return None + + +@pytest.mark.asyncio +async def test_filter_error_is_audited_as_deny(monkeypatch): + executor, _ = _executor() + plan = _plan(executor) + audited = [] + policy = ReviewPolicyFilter(lambda items: _capture(audited, items)) + + def fail(value): + del value + raise RuntimeError("filter failure") + + monkeypatch.setattr(policy, "evaluate", fail) + decisions = await policy.evaluate_and_audit(plan) + + assert decisions[0].rule_id == "filter.internal-error" + assert decisions[0].action == DecisionAction.DENY + assert audited == decisions + + +async def _capture(target, values): + target.extend(values) + + +@pytest.mark.asyncio +async def test_sandbox_stages_only_fixed_files_and_cleans_up(): + finding = ('{"category":"security","confidence":0.9,"evidence":"safe",' + '"file":"app.py","line":1,"recommendation":"fix",' + '"severity":"high","source":"test","title":"issue"}\n') + executor, runtime = _executor(FakeRuntime(filesystem=FakeFS(finding))) + result = await executor.execute(_plan(executor), INPUT_BYTES) + + assert result.run.status == SandboxStatus.SUCCEEDED + assert result.output == finding + assert runtime._manager.cleanup_calls == 1 + assert runtime._filesystem.put_paths == [ + "skills/code-review/SKILL.md", + "skills/code-review/references/rules.md", + "skills/code-review/scripts/scan_rules.py", + "work/inputs/review_input.json", + ] + + +@pytest.mark.asyncio +async def test_success_redaction_preserves_combined_output_limit(): + secret = 'api_key="abcdefgh"' + executor, _ = _executor(FakeRuntime(runner=FakeRunner(stdout=secret))) + limit = len(secret.encode("utf-8")) + plan = _plan(executor).model_copy(update={"output_limit_bytes": limit, "digest": "pending"}) + plan = plan.model_copy(update={"digest": calculate_plan_digest(plan)}) + + result = await executor.execute(plan, INPUT_BYTES) + + combined = result.run.stdout + result.run.stderr + result.output + assert "abcdefgh" not in combined + assert len(combined.encode("utf-8")) <= limit + + +@pytest.mark.asyncio +async def test_sandbox_timeout_cleans_up(): + runtime = FakeRuntime(runner=FakeRunner(delay=0.05)) + executor, runtime = _executor(runtime) + plan = _plan(executor).model_copy(update={ + "timeout_seconds": 0.01, + "output_limit_bytes": 10, + "digest": "pending", + }, ) + plan = plan.model_copy(update={"digest": calculate_plan_digest(plan)}) + + result = await executor.execute(plan, INPUT_BYTES) + + assert result.run.status == SandboxStatus.TIMED_OUT + assert result.run.timed_out is True + assert runtime._manager.cleanup_calls == 1 + + +@pytest.mark.asyncio +async def test_cleanup_failure_is_recorded_after_timeout(): + manager = FakeManager(cleanup_error=True) + runtime = FakeRuntime(manager=manager, runner=FakeRunner(delay=0.05)) + executor, _ = _executor(runtime) + plan = _plan(executor).model_copy(update={"timeout_seconds": 0.01, "digest": "pending"}, ) + plan = plan.model_copy(update={"digest": calculate_plan_digest(plan)}) + + result = await executor.execute(plan, INPUT_BYTES) + + assert result.run.status == SandboxStatus.TIMED_OUT + assert result.run.error_type == "TimeoutError+RuntimeError" + output_size = len((result.run.stdout + result.run.stderr + result.output).encode("utf-8"), ) + assert output_size <= plan.output_limit_bytes + + +@pytest.mark.asyncio +async def test_sandbox_output_cap_is_enforced(): + output = "x" * 100 + executor, _ = _executor(FakeRuntime(filesystem=FakeFS(output))) + plan = _plan(executor).model_copy(update={"output_limit_bytes": 10, "digest": "pending"}, ) + plan = plan.model_copy(update={"digest": calculate_plan_digest(plan)}) + + result = await executor.execute(plan, INPUT_BYTES) + + assert result.run.status == SandboxStatus.FAILED + assert result.run.error_type == "ValueError" + + +def test_local_runtime_requires_explicit_opt_in(monkeypatch): + from examples.skills_code_review_agent.agent.sandbox import create_runtime + + monkeypatch.delenv("TRPC_CODE_REVIEW_ALLOW_UNSAFE_LOCAL", raising=False) + with pytest.raises(PermissionError): + create_runtime("local") + + +def test_container_runtime_explicitly_disables_network(monkeypatch): + from examples.skills_code_review_agent.agent import sandbox + + captured = {} + + def create(**kwargs): + captured.update(kwargs) + return FakeRuntime() + + monkeypatch.setattr(sandbox, "create_container_workspace_runtime", create) + handle = sandbox.create_runtime("container") + + assert captured["host_config"]["network_mode"] == "none" + assert handle.network_disabled is True diff --git a/tests/examples/skills_code_review_agent/test_reporting.py b/tests/examples/skills_code_review_agent/test_reporting.py new file mode 100644 index 000000000..38d48415b --- /dev/null +++ b/tests/examples/skills_code_review_agent/test_reporting.py @@ -0,0 +1,162 @@ +"""Tests for finding validation, deduplication, and report output.""" + +from __future__ import annotations + +from datetime import datetime +from datetime import timezone +import json + +import pytest + +from examples.skills_code_review_agent.agent.models import Category +from examples.skills_code_review_agent.agent.models import Finding +from examples.skills_code_review_agent.agent.models import ReviewMetrics +from examples.skills_code_review_agent.agent.models import ReviewReport +from examples.skills_code_review_agent.agent.models import Severity +from examples.skills_code_review_agent.agent.models import TaskStatus +from examples.skills_code_review_agent.agent.policy import SecretRedactor +from examples.skills_code_review_agent.agent.reporting import FindingOutputError +from examples.skills_code_review_agent.agent.reporting import deduplicate_findings +from examples.skills_code_review_agent.agent.reporting import parse_findings_jsonl +from examples.skills_code_review_agent.agent.reporting import prepare_findings +from examples.skills_code_review_agent.agent.reporting import render_markdown +from examples.skills_code_review_agent.agent.reporting import write_report_files + +SECRET = "s" + "k-1234567890abcdefghijklmnop" +GENERIC_SECRET = "CorrectHorseBatteryStaple" + + +def _finding(**updates) -> Finding: + values = { + "severity": Severity.MEDIUM, + "category": Category.SECURITY, + "file": "app.py", + "line": 3, + "title": "Unsafe call", + "evidence": "eval(value)", + "recommendation": "Parse allowed values.", + "confidence": 0.9, + "source": "rule.security.eval", + } + values.update(updates) + return Finding.model_validate(values) + + +def _report(**updates) -> ReviewReport: + values = { + "task_id": + "task-report", + "status": + TaskStatus.COMPLETE, + "input_summary": { + "file_count": 1 + }, + "findings": [_finding()], + "warnings": [], + "needs_human_review": [], + "filter_decisions": [], + "sandbox_runs": [], + "failures": [], + "metrics": + ReviewMetrics( + total_duration_ms=1, + sandbox_duration_ms=1, + tool_calls=2, + blocked_count=0, + finding_count=1, + ), + "conclusion": + "Review complete.", + "created_at": + datetime.now(timezone.utc), + } + values.update(updates) + return ReviewReport.model_validate(values) + + +def test_parse_findings_requires_exact_contract() -> None: + payload = _finding().model_dump(mode="json") + assert parse_findings_jsonl(json.dumps(payload), SecretRedactor()) == [_finding()] + payload["unexpected"] = True + with pytest.raises(FindingOutputError, match="exactly"): + parse_findings_jsonl(json.dumps(payload), SecretRedactor()) + + +@pytest.mark.parametrize( + "text,error", + [ + ("not json", "invalid finding JSON"), + (json.dumps({"nested": [[[[[[[[[[]]]]]]]]]]}), "exactly"), + ], +) +def test_parse_findings_rejects_invalid_json(text: str, error: str) -> None: + with pytest.raises(FindingOutputError, match=error): + parse_findings_jsonl(text, SecretRedactor()) + + +def test_deduplicate_merges_best_finding_and_sources() -> None: + low = _finding(confidence=0.7, source="rule.one", evidence="first") + high = _finding( + severity=Severity.HIGH, + confidence=0.95, + source="rule.two", + evidence="second", + ) + result = deduplicate_findings([low, high]) + assert len(result) == 1 + assert result[0].severity == Severity.HIGH + assert result[0].confidence == 0.95 + assert result[0].source == "rule.one | rule.two" + assert result[0].evidence == "first | second" + + +def test_low_confidence_becomes_warning_and_human_review() -> None: + buckets = prepare_findings( + [_finding(confidence=0.2, evidence=SECRET)], + SecretRedactor(), + ) + assert not buckets.actionable + assert buckets.warnings[0].severity == Severity.WARNING + assert buckets.needs_human_review == buckets.warnings + assert SECRET not in buckets.warnings[0].evidence + + +def test_render_markdown_has_fixed_sections() -> None: + markdown = render_markdown(_report()) + for heading in ( + "## Task summary", + "## Findings", + "## Warnings / needs human review", + "## Filter decisions", + "## Sandbox runs", + "## Exceptions", + "## Metrics", + "## Final conclusion", + ): + assert heading in markdown + + +def test_render_markdown_escapes_untrusted_finding_text() -> None: + evidence = "\n## forged heading" + markdown = render_markdown(_report(findings=[_finding(evidence=evidence)])) + + assert "