Skip to content

Commit aaad528

Browse files
coder-mtjcoder-mtj
committed
feat(code-review-agent): upgrade pipeline with 205 tests, 10 scanners, 3-tier confidence, AST analysis, SARIF output, schema versioning, and full documentation
Bug fixes: - Fix report.py severity/category confusion (secret_info is a category, not severity) - Fix sandbox script path resolution to locate skills/ from repo root - Fix run_review.py emoji to ASCII-safe for Windows GBK compatibility - Fix agent.py hardcoded model → env-var-driven configuration - Fix SKILL.md missing references (create OUTPUT_SCHEMA.md, rules/rules.json) - Fix run_checks.py from stub to functional scanner execution Feature enhancements: - Three-tier confidence system (high >=0.8 / warning >=0.55 / needs_human_review) - Policy-as-code filter governance (filter_policy.json) - FakeSandboxRunner with trigger-based edge case simulation - AST taint analysis (Python AST + JS/TS regex) - 4 new scanners: bare_except, mutable_defaults, assert_control_flow, hardcoded_paths - Per-scanner confidence thresholds - Schema versioning with incremental column migrations (v1→v4) - SARIF v2.1.0 output for GitHub Code Scanning - Fixture evaluation framework with precision/recall/F1 - Multi-input support (--diff-file, --repo-path, stdin) Test expansion (116 → 205 tests, 9 → 15 files): - test_agent.py (8), test_ast_analyzer.py (18), test_cli.py (12) - test_edge_cases.py (22), test_performance.py (8), test_fixture_evaluation.py (10) Documentation: - DESIGN.md (159 lines): architecture, design decisions, trade-offs - README.md: bilingual, CLI reference, scan categories table - ai-prompts.md: 4-round development record
1 parent 19ade39 commit aaad528

26 files changed

Lines changed: 3066 additions & 180 deletions

‎examples/skills_code_review_agent/DESIGN.md‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
# Code Review Agent — Architecture Design
2+
3+
## Overview
4+
5+
基于 tRPC-Agent SDK 的自动化代码评审 Agent，集成 Skills、沙箱执行、SQLite 存储，提供端到端的代码审查流水线。
6+
7+
**核心理念**：将代码评审拆解为独立的流水线阶段，每个阶段可独立测试、替换和扩展。
8+
9+
## Architecture
10+
11+
### Pipeline (8-stage linear workflow)
12+
13+
```
14+
┌──────────┐ ┌──────────┐ ┌───────────────┐ ┌──────────┐
15+
│ 1. Read │ → │ 2. Parse │ → │ 3. Filter │ → │ 4. Scan │
16+
│ diff │ │ diff │ │ chain │ │ code │
17+
└──────────┘ └──────────┘ └───────────────┘ └──────────┘
18+
│ │ │ │
19+
▼ ▼ ▼ ▼
20+
--diff-file unified diff SafetyFilter x4 10 scanners
21+
--repo-path DiffFile[] deny/allow/needs Finding[]
22+
--stdin DiffHunk[] _human_review
23+
24+
┌──────────┐ ┌──────────┐ ┌──────────┐ ┌──────────┐
25+
│ 5. Sand- │ → │ 6. Dedup │ → │ 7. Report│ → │ 8. Store │
26+
│ box │ │ +Redact │ │ gen │ │ DB │
27+
└──────────┘ └──────────┘ └──────────┘ └──────────┘
28+
│ │ │ │
29+
▼ ▼ ▼ ▼
30+
Fake/Local/ fingerprint JSON + MD SQLite
31+
Workspace + 3-tier conf + SARIF + schema
32+
SandboxRun + 12 patterns ReviewReport versioning
33+
```
34+
35+
### Module Map
36+
37+
| Module | File | Lines | Responsibility |
38+
|--------|------|-------|----------------|
39+
| Types | `pipeline/types.py` | ~117 | Data contracts (Finding, DiffFile, SandboxRun, etc.) |
40+
| Config | `pipeline/config.py` | ~52 | Pipeline configuration with defaults and overrides |
41+
| Diff Parser | `pipeline/diff_parser.py` | ~185 | Unified diff parsing, changed line extraction |
42+
| Filter Chain | `pipeline/filter_chain.py` | ~135 | Safety filter chain with policy-as-code support |
43+
| Scanners | `pipeline/scanners.py` | ~340 | 10 pattern-matching scanners with per-category thresholds |
44+
| Sandbox | `pipeline/sandbox.py` | ~220 | Fake/Local/Workspace sandbox abstraction |
45+
| Dedup | `pipeline/dedup.py` | ~110 | Fingerprint-based dedup with 3-tier confidence |
46+
| Redaction | `pipeline/redaction.py` | ~82 | 12 regex patterns for secret/credential redaction |
47+
| AST Analyzer | `pipeline/ast_analyzer.py` | ~230 | Python AST + JS/TS regex taint analysis |
48+
| Report | `pipeline/report.py` | ~210 | JSON + Markdown report generation |
49+
| SARIF Output | `pipeline/sarif_output.py` | ~120 | SARIF v2.1.0 for GitHub Code Scanning integration |
50+
| Telemetry | `pipeline/telemetry.py` | ~78 | Timing and cost collection |
51+
| Storage/Models | `storage/models.py` | ~72 | DB record dataclasses |
52+
| Storage/DAO | `storage/dao.py` | ~320 | SQLite CRUD with schema versioning and migrations |
53+
| Agent | `agent/agent.py` | ~50 | LlmAgent wrapper for tRPC-Agent framework |
54+
| Skill | `skills/code-review/SKILL.md` | ~43 | Skill definition with rules and scripts |
55+
| CLI | `run_review.py` | ~360 | argparse CLI entry point |
56+
| Fixture Eval | `evaluate_fixtures.py` | ~200 | Precision/recall/F1 evaluation framework |
57+
58+
## Key Design Decisions
59+
60+
### 1. Pattern-Match Scanners vs ML-based Analysis
61+
62+
**Decision**: Use regex-based pattern matching with AST enhancement.
63+
**Rationale**:
64+
- Deterministic and fast — no API calls, no rate limits
65+
- Easy to audit and extend — adding a rule is one tuple
66+
- AST analysis adds semantic understanding without complexity
67+
- 10 scanners covering security, async, resources, DB, tests, secrets, and code quality
68+
69+
### 2. Three-Tier Confidence System
70+
71+
**Decision**: Classify findings into high (≥0.8), warning (≥0.55), needs_human_review (<0.55).
72+
**Rationale**:
73+
- Mirrors real production review workflows
74+
- High-confidence findings can auto-block merges
75+
- Warning tier gives reviewers a prioritized list
76+
- Low-confidence items don't create noise in automated mode
77+
- Per-scanner threshold overrides allow fine-tuning
78+
79+
### 3. Multi-Runtime Sandbox Abstraction
80+
81+
**Decision**: SandboxRunner ABC with Fake/Local/Workspace implementations.
82+
**Rationale**:
83+
- Fake runner enables complete CI testing without Docker
84+
- Local runner for development with subprocess isolation
85+
- Workspace runner for production (Container/Cube/E2B)
86+
- Trigger-based edge case simulation in fake mode (timeout, large output, secrets)
87+
88+
### 4. Policy-as-Code Filter Governance
89+
90+
**Decision**: Externalize filter rules to `filter_policy.json`.
91+
**Rationale**:
92+
- Rules updateable without code changes
93+
- Auditable — filter decisions logged to DB
94+
- Extensible — add new patterns without redeploying
95+
- Pre-block high-risk scripts before sandbox execution
96+
97+
### 5. SQLite with Schema Versioning
98+
99+
**Decision**: Use SQLite with COLUMN_MIGRATIONS dict for schema evolution.
100+
**Rationale**:
101+
- Zero-dependency storage
102+
- Schema version table tracks current state
103+
- Column migrations apply incrementally (v1→v2→v3→v4)
104+
- WAL mode for concurrent reads
105+
- Forward-compatible: new columns have defaults
106+
107+
### 6. Multi-Format Output (JSON + MD + SARIF)
108+
109+
**Decision**: Generate three output formats from the same ReviewReport.
110+
**Rationale**:
111+
- JSON: machine-readable, API integration
112+
- Markdown: human-readable, PR comments
113+
- SARIF v2.1.0: GitHub Code Scanning, Azure DevOps integration
114+
115+
## Failure Modes and Mitigations
116+
117+
| Failure Mode | Detection | Mitigation |
118+
|-------------|-----------|------------|
119+
| Empty diff | Stage 2: 0 files parsed → early exit | No crash, clean exit 0 |
120+
| Malicious diff content | Stage 3: FilterChain evaluation | Block before sandbox execution |
121+
| Sandbox timeout | Stage 5: timeout_seconds limit | SandboxRun.timed_out=True, pipeline continues |
122+
| Sandbox crash | Stage 5: exception handling | SandboxRun.error, pipeline continues |
123+
| Output too large | Stage 5: max_output_bytes limit | Truncation with marker |
124+
| DB connection failure | Stage 8: exception handling | Report still written to disk before DB |
125+
| Scanner regex catastrophic backtracking | Per-scanner execution, timeout | Individual scanner failure doesn't crash others |
126+
127+
## Trade-offs
128+
129+
| Trade-off | Choice | Why |
130+
|-----------|--------|-----|
131+
| Speed vs Accuracy | Prioritized speed | Regex scanners are O(n) per pattern; real LLM review can be layered on top |
132+
| Coverage vs Precision | Balanced (3-tier) | High-confidence blocking + human review for edge cases |
133+
| Simplicity vs Features | Leaned toward feature completeness | 10 scanners, 3 sandbox modes, 3 output formats, but each module is simple |
134+
| Testing vs Implementation | Heavy testing investment | 205 tests across 15 files → refactoring confidence |
135+
136+
## Data Flow
137+
138+
```
139+
diff_text (str)
140+
→ parse_diff() → DiffFile[]
141+
→ FilterChain.evaluate() → FilterDecision
142+
→ run_scanners() × N → Finding[]
143+
→ FakeSandboxRunner.run() → SandboxRun
144+
→ deduplicate() → Finding[] (deduped)
145+
→ separate_by_tiers() → {high, warning, needs_human_review}
146+
→ redact_finding_evidence() → Finding[] (redacted)
147+
→ generate_json_report() → JSON string
148+
→ generate_md_report() → Markdown string
149+
→ generate_sarif() → SARIF JSON string
150+
→ ReviewDatabase.insert_*() → SQLite rows
151+
```
152+
153+
## Extensibility
154+
155+
1. **Add a scanner**: Create function `scan_xxx(diff_file) → list[Finding]`, add to `_SCANNERS` dict
156+
2. **Add a filter**: Add to `filter_policy.json` or create SafetyFilter in code
157+
3. **Add a sandbox**: Implement `SandboxRunner` ABC, register in `create_sandbox_runner()`
158+
4. **Add an output format**: Create `generate_xxx(report) → str`, call from `run_review.py`
159+
5. **Schema migration**: Add entry to `COLUMN_MIGRATIONS` dict, increment `SCHEMA_VERSION`

0 commit comments

Comments
 (0)