Skip to content

Commit d829835

Browse files
committed
fix(code-review-agent): align sandbox outputs and portable reports
1 parent 77078a4 commit d829835

11 files changed

Lines changed: 351 additions & 66 deletions

File tree

examples/skills_code_review_agent/DESIGN_NOTE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
本方案将自动代码评审拆为“主流程编排 + 可复用 Skill + 受控执行 + 结构化落库”四层。主流程由 `agent/agent.py` 负责,统一接收 diff、repo path 或 fixture,完成输入归一化、diff 解析、规则执行、Filter 决策、skill 脚本调度、报告生成和 SQLite 持久化。`skills/code-review/` 则承载正式的 `code-review` Skill,包括 `SKILL.md`、规则文档、使用文档、脚本契约与三个确定性脚本,用于承接可复用的评审知识与脚本执行面。
44

5-
沙箱隔离策略采用“默认受控脚本执行 + 本地 fallback + 容器接口预留”的实现方式。当前示例通过统一的脚本执行层提供 timeout、输出截断、失败记录和 Filter 前置治理,确保高风险脚本、禁止路径、默认网络访问和超预算输入不能直接进入执行链路。对脚本失败或超时,系统不会整体崩溃,而是转换为可追踪的 `sandbox_runs` 记录和结构化 finding。
5+
沙箱隔离策略采用“显式本地开发回退 + 容器/远端接口预留”的实现方式。当前示例只有 `local` 会实际执行脚本,`container``cube``e2b` 等 runtime 在未接入真实隔离执行器前,都会先被 Filter 标记为 `needs_human_review`,避免把宿主机执行误报成生产安全沙箱。统一脚本执行层仍然提供 timeout、输出截断、失败记录和 Filter 前置治理,确保高风险脚本、禁止路径、默认网络访问和超预算输入不能直接进入执行链路。对脚本失败或超时,系统不会整体崩溃,而是转换为可追踪的 `sandbox_runs` 记录和结构化 finding。
66

77
数据库 schema 采用最小可查询设计,包含 `review_tasks``review_inputs``filter_decisions``sandbox_runs``findings``review_reports` 六张表,支持按 `task_id` 查询完整审查链路。报告输出同时生成 JSON 与 Markdown,两者都包含 findings 摘要、人工复核项、Filter 摘要、sandbox 摘要和监控指标。监控字段聚合总耗时、severity/category 分布、拦截次数和 sandbox 次数,便于回放和评测。
88

examples/skills_code_review_agent/README.md

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
This example implements a structured code-review agent prototype on top of
44
tRPC-Agent-Python. It combines deterministic rule detection, a formal
5-
`code-review` skill package, filter-based governance, sandboxed script
5+
`code-review` skill package, filter-based governance, development-local script
66
execution, SQLite persistence, and dual-format reports.
77

88
## What This Example Covers
@@ -17,7 +17,7 @@ execution, SQLite persistence, and dual-format reports.
1717
- database lifecycle issues
1818
- Finding dedupe and confidence-based routing
1919
- Filter decisions before sandbox execution
20-
- Sandboxed skill-script execution with timeout and output truncation
20+
- Development-local skill-script execution with timeout and output truncation
2121
- Secret redaction before reporting and persistence
2222
- SQLite storage for tasks, inputs, findings, sandbox runs, filter decisions, and reports
2323
- `review_report.json` and `review_report.md` output generation
@@ -43,7 +43,7 @@ CLI / input
4343
-> deterministic rule engine
4444
-> dedupe and confidence routing
4545
-> filter decisions
46-
-> sandboxed skill scripts
46+
-> governed skill scripts
4747
-> redaction
4848
-> JSON/Markdown reports
4949
-> SQLite persistence
@@ -58,6 +58,7 @@ python examples/skills_code_review_agent/run_agent.py ^
5858
--fixture examples/skills_code_review_agent/tests/fixtures/security_issue.diff ^
5959
--output-dir examples/skills_code_review_agent/sample_outputs ^
6060
--db-path examples/skills_code_review_agent/sample_outputs/review.db ^
61+
--runtime local ^
6162
--dry-run ^
6263
--fake-model
6364
```
@@ -69,6 +70,7 @@ python examples/skills_code_review_agent/run_agent.py ^
6970
--diff-file path/to/change.diff ^
7071
--output-dir examples/skills_code_review_agent/sample_outputs ^
7172
--db-path examples/skills_code_review_agent/sample_outputs/review.db ^
73+
--runtime local ^
7274
--dry-run ^
7375
--fake-model
7476
```
@@ -80,10 +82,16 @@ python examples/skills_code_review_agent/run_agent.py ^
8082
--repo-path path/to/repo ^
8183
--output-dir examples/skills_code_review_agent/sample_outputs ^
8284
--db-path examples/skills_code_review_agent/sample_outputs/review.db ^
85+
--runtime local ^
8386
--dry-run ^
8487
--fake-model
8588
```
8689

90+
`local` is the only runtime that currently executes scripts in this example,
91+
and it should be treated as a development fallback. Requests for `container`,
92+
`cube`, or `e2b` are recorded by Filter as `needs_human_review` until a real
93+
isolated executor is wired in.
94+
8795
## Outputs
8896

8997
Each run produces:
@@ -115,9 +123,7 @@ The repository layer persists these tables:
115123
- `findings`
116124
- `review_reports`
117125

118-
You can fetch a full task bundle with:
119-
120-
- [ReviewRepository](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/src/storage/repository.py)
126+
You can fetch a full task bundle with `src/storage/repository.py`.
121127

122128
Key method:
123129

@@ -127,13 +133,13 @@ Key method:
127133

128134
The canonical reusable skill lives under:
129135

130-
- [SKILL.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/SKILL.md)
131-
- [USAGE.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/USAGE.md)
132-
- [RULES.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/RULES.md)
133-
- [SCRIPT_CONTRACTS.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/SCRIPT_CONTRACTS.md)
136+
- `skills/code-review/SKILL.md`
137+
- `skills/code-review/USAGE.md`
138+
- `skills/code-review/RULES.md`
139+
- `skills/code-review/SCRIPT_CONTRACTS.md`
134140

135141
The example now resolves the repository-level `skills/code-review/` directory
136-
first for repository indexing, skill-script planning, and container skill mounts.
142+
first for repository indexing, skill-script planning, and future isolated runtime mounts.
137143
The example-local copy remains as a fallback so the sample stays readable and
138144
self-contained.
139145

@@ -169,7 +175,7 @@ Phase 6 quality-gate evidence:
169175

170176
## Related Docs
171177

172-
- [DEVELOPMENT_PLAN.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/DEVELOPMENT_PLAN.md)
173-
- [DESIGN_NOTE.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/DESIGN_NOTE.md)
174-
- [ACCEPTANCE_CHECKLIST.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/ACCEPTANCE_CHECKLIST.md)
175-
- [PR_READINESS.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/PR_READINESS.md)
178+
- `DEVELOPMENT_PLAN.md`
179+
- `DESIGN_NOTE.md`
180+
- `ACCEPTANCE_CHECKLIST.md`
181+
- `PR_READINESS.md`

examples/skills_code_review_agent/agent/agent.py

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from __future__ import annotations
44

5+
import json
56
from dataclasses import dataclass
67
from datetime import datetime, timezone
78
from pathlib import Path
@@ -18,8 +19,11 @@
1819
from ..src.report_writer import build_report_payload, render_markdown_report, write_report_files
1920
from ..src.rule_engine import run_rule_engine
2021
from ..src.review_types import (
22+
DiffLineType,
2123
FindingDisposition,
2224
FindingSource,
25+
FilterDecisionRecord,
26+
FilterDecisionType,
2327
ReviewCategory,
2428
ReviewConclusion,
2529
ReviewFinding,
@@ -91,6 +95,7 @@ def run_review_task(config: ReviewAgentConfig) -> tuple[ReviewTask, ReviewReport
9195
sandbox_run = execute_skill_script(invocation, runtime=config.runtime)
9296
task.add_sandbox_run(sandbox_run)
9397
all_findings.extend(_sandbox_run_findings(task, sandbox_run))
98+
all_findings.extend(_sandbox_output_findings(task, sandbox_run))
9499
else:
95100
task.add_sandbox_run(
96101
build_blocked_run(
@@ -99,6 +104,7 @@ def run_review_task(config: ReviewAgentConfig) -> tuple[ReviewTask, ReviewReport
99104
reason=decision.reason,
100105
)
101106
)
107+
all_findings.extend(_filter_decision_findings(task, decision))
102108

103109
processed_findings = dedupe_and_classify_findings(all_findings)
104110
for finding in processed_findings:
@@ -206,3 +212,146 @@ def _sandbox_run_findings(task: ReviewTask, sandbox_run) -> list[ReviewFinding]:
206212
source=FindingSource.SANDBOX,
207213
)
208214
]
215+
216+
217+
def _sandbox_output_findings(task: ReviewTask, sandbox_run) -> list[ReviewFinding]:
218+
"""Promote successful skill-script diagnostics into structured findings."""
219+
220+
if sandbox_run.status != sandbox_run.status.SUCCEEDED or not sandbox_run.stdout:
221+
return []
222+
223+
try:
224+
payload = json.loads(sandbox_run.stdout)
225+
except json.JSONDecodeError:
226+
return []
227+
228+
if sandbox_run.name != "run_linters":
229+
return []
230+
231+
findings: list[ReviewFinding] = []
232+
for warning in payload.get("warnings", []):
233+
mapping = _linter_warning_to_finding(task, warning)
234+
if mapping is not None:
235+
findings.append(mapping)
236+
return findings
237+
238+
239+
def _linter_warning_to_finding(task: ReviewTask, warning: str) -> ReviewFinding | None:
240+
"""Map deterministic linter warnings onto the canonical finding schema."""
241+
242+
warning_map = {
243+
"Security-sensitive call detected: eval": {
244+
"needle": "eval(",
245+
"severity": ReviewSeverity.HIGH,
246+
"title": "Use of eval introduces code execution risk",
247+
"recommendation": (
248+
"Replace eval with explicit parsing, a whitelist-based dispatcher, "
249+
"or a safe literal parser."
250+
),
251+
"confidence": 0.99,
252+
},
253+
"Shell execution enabled in subprocess call": {
254+
"needle": "shell=True",
255+
"severity": ReviewSeverity.HIGH,
256+
"title": "subprocess call enables shell execution",
257+
"recommendation": (
258+
"Pass an argument list and avoid shell=True unless a reviewed shell command "
259+
"is unavoidable."
260+
),
261+
"confidence": 0.96,
262+
},
263+
"TLS verification disabled": {
264+
"needle": "verify=False",
265+
"severity": ReviewSeverity.MEDIUM,
266+
"title": "TLS certificate verification is disabled",
267+
"recommendation": (
268+
"Keep certificate verification enabled or document a controlled "
269+
"test-only exception."
270+
),
271+
"confidence": 0.89,
272+
},
273+
}
274+
mapped = warning_map.get(warning)
275+
if mapped is None:
276+
return None
277+
278+
file_path, line_number, evidence = _find_added_line_evidence(
279+
task,
280+
needle=mapped["needle"],
281+
)
282+
return ReviewFinding(
283+
severity=mapped["severity"],
284+
category=ReviewCategory.SECURITY,
285+
file=file_path,
286+
line=line_number,
287+
title=mapped["title"],
288+
evidence=evidence,
289+
recommendation=mapped["recommendation"],
290+
confidence=mapped["confidence"],
291+
source=FindingSource.SKILL_SCRIPT,
292+
)
293+
294+
295+
def _filter_decision_findings(
296+
task: ReviewTask,
297+
decision: FilterDecisionRecord,
298+
) -> list[ReviewFinding]:
299+
"""Expose non-allow filter decisions in the final report."""
300+
301+
if decision.decision == FilterDecisionType.ALLOW:
302+
return []
303+
304+
severity = ReviewSeverity.MEDIUM
305+
title = "Sandbox invocation requires human review"
306+
recommendation = (
307+
"Use local runtime only for explicit development fallback, or implement a real "
308+
"isolated executor before allowing non-local runtimes."
309+
)
310+
if decision.decision == FilterDecisionType.DENY:
311+
severity = ReviewSeverity.HIGH
312+
title = "Sandbox invocation was denied by filter policy"
313+
recommendation = "Adjust the diff or invocation so it complies with the sandbox policy."
314+
315+
file_path = (
316+
task.parsed_diff.changed_paths[0]
317+
if task.parsed_diff and task.parsed_diff.changed_paths
318+
else task.review_input.source
319+
)
320+
return [
321+
ReviewFinding(
322+
severity=severity,
323+
category=ReviewCategory.SANDBOX,
324+
file=file_path,
325+
line=None,
326+
title=title,
327+
evidence=f"{decision.target}: {decision.reason}",
328+
recommendation=recommendation,
329+
confidence=0.9 if decision.decision == FilterDecisionType.DENY else 0.75,
330+
source=FindingSource.FILTER,
331+
disposition=FindingDisposition.NEEDS_HUMAN_REVIEW,
332+
)
333+
]
334+
335+
336+
def _find_added_line_evidence(
337+
task: ReviewTask,
338+
*,
339+
needle: str,
340+
) -> tuple[str, int | None, str]:
341+
"""Locate the added line that triggered a sandbox warning."""
342+
343+
if task.parsed_diff is None:
344+
return task.review_input.source, None, needle
345+
346+
for changed_file in task.parsed_diff.files:
347+
for hunk in changed_file.hunks:
348+
for line in hunk.lines:
349+
if line.line_type != DiffLineType.ADD:
350+
continue
351+
if needle not in line.text:
352+
continue
353+
return changed_file.display_path, line.new_line_no, line.raw_line
354+
355+
if task.parsed_diff.changed_paths:
356+
return task.parsed_diff.changed_paths[0], None, needle
357+
return task.review_input.source, None, needle

0 commit comments

Comments
 (0)