Skip to content

feat: add Skills-based sandboxed code review agent#236

Open
mingri31164 wants to merge 1 commit into
trpc-group:mainfrom
mingri31164:feat/issue-92-skills-code-review-agent
Open

feat: add Skills-based sandboxed code review agent#236
mingri31164 wants to merge 1 commit into
trpc-group:mainfrom
mingri31164:feat/issue-92-skills-code-review-agent

Conversation

@mingri31164

@mingri31164 mingri31164 commented Jul 25, 2026

Copy link
Copy Markdown

Summary

  • add a reusable code-review Skill with documented and machine-readable rules for security, async errors, resource leaks, test coverage, sensitive data, and database lifecycles
  • parse Git/standard unified diffs, file lists, normal repositories, linked worktrees, tracked changes, and untracked files
  • run deterministic checks through the SDK's skill_load and skill_run flow; Container with no network is the production default and Local is an explicit development fallback
  • persist review tasks, sandbox runs, Filter decisions, deduplicated findings, monitoring summaries, and final reports through SqlStorage
  • emit redacted review_report.json and review_report.md with findings, human-review items, Filter/sandbox summaries, metrics, and actionable recommendations

Safety and governance

  • execute the Filter before runtime/workspace creation
  • deny unsafe executables, shell operators, path traversal, forbidden paths, non-allowlisted network targets, and environment variables
  • route timeout/output budget overruns to needs_human_review without starting a sandbox
  • enforce timeout, output size limits, read-only input mounts, disabled provider environment forwarding, and network_mode=none
  • redact API keys, bearer/provider tokens, passwords (including quoted JSON and values with spaces), URL credentials, JWT/AWS patterns, and private-key material in scanner output, host parsing, reports, queries, and SQL writes
  • deduplicate by (file, line, category) and route confidence below 0.80 away from primary findings

Validation

  • 25 passed: example unit/integration suite, including a real Docker Container E2E
  • 374 passed: existing Skill load/run, Filter, SQL storage, and Local/Container workspace regression suites
  • public fixtures: 8/8 generated JSON and Markdown reports in about 5.4s in fake-model Local mode
  • production-default Container run: completed in 11.44s, skill_load=True, skill_run=True, 7 structured findings, 21 redactions, and no residual container/workspace
  • SQLite query verified task, sandbox run, Filter decision, 7 findings, monitoring summary, and report for the same task
  • plaintext scan across generated JSON, Markdown, SQLite, and captured logs: 0 matches for all fixture credentials
  • Flake8, Black check, compileall, git diff --check, JSON validation, and pip check passed
  • repository CI: GitHub build, lint, and test, Codecov patch coverage, CLA, and open-source scan passed

Runtime evidence

Default Container execution, read-only bind mount, Skill flow, cleanup, and report summary:

Container sandbox E2E

SQLite persistence and redaction audit:

SQLite persistence and redaction

Release note

Added a production-oriented Skills code review agent example with sandbox governance, SQL persistence, structured reports, monitoring, deduplication, and defense-in-depth credential redaction.

Fixes #92

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mingri31164

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

Rook1ex added a commit to trpc-group/cla-database that referenced this pull request Jul 25, 2026
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@6a2f7f9). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             main        #236   +/-   ##
==========================================
  Coverage        ?   88.07937%           
==========================================
  Files           ?         482           
  Lines           ?       45157           
  Branches        ?           0           
==========================================
  Hits            ?       39774           
  Misses          ?        5383           
  Partials        ?           0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mingri31164
mingri31164 force-pushed the feat/issue-92-skills-code-review-agent branch from f9212a0 to 765f9e9 Compare July 25, 2026 18:11
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

我已经完整阅读了 pr.diff(新增的 examples/skills_code_review_agent/ 示例及 tests/examples/test_skills_code_review_agent.py)。下面是审查结论。

发现的问题

⚠️ Warning

  • tests/examples/test_skills_code_review_agent.py:340-346:生产默认的 container 运行时在 CI 中不被覆盖

    • 唯一一条 container E2E 用例被 @pytest.mark.skipif(os.getenv("RUN_CODE_REVIEW_CONTAINER_TEST") != "1", ...) 门控,默认跳过;其余所有用例都走 runtime="local" 这一"显式开发回退"路径。而 ReviewConfig.runtime 默认值和 run_agent.py 默认都是 container,意味着生产默认路径(host bind mount、DEFAULT_INPUTS_CONTAINER 映射、容器内 output_files 取回等容器特有逻辑)在常规 CI 中完全没有被验证,容器侧回归只会在线上或手动设置环境变量时才暴露。建议在 CI 中加入一条门控触发 container 用例的 job,或至少把 container E2E 纳入可定期运行的矩阵。
  • examples/skills_code_review_agent/run_agent.py:62-64examples/skills_code_review_agent/agent/workflow.py:55-56:仅暴露 --max-output-bytes,提高预算反而触发 output_budget 拦截

    • CLI 只能调 --max-output-bytes(同时作用于实际输出捕获和 tool_args.outputsmax_total_bytes/max_file_bytes),而 Filter 的策略上限 max_policy_output_bytes 始终保持默认 64K。当用户为避免输出截断而调大 --max-output-bytes 时,governance.pyoutput_budget 判定会因 max_total > max_policy_output_bytes 直接 needs_human_review 并阻止沙箱启动(对应 test_over_budget_output_is_blocked_before_sandbox)。这是反直觉的兼容性/可用性陷阱,建议同步暴露 --max-policy-output-bytes,或在 CLI 层将两者绑定推导。

💡 Suggestion

  • examples/skills_code_review_agent/agent/workflow.py:189-190finallystaged_diff.unlink(missing_ok=True) 后直接 staged_diff.parent.rmdir() 未做异常隔离,若 inputs/<task_id> 目录因任何原因非空(例如未来 SDK 行为变化写入额外文件),rmdir 抛出的 OSError 会从 finally 传播并覆盖已生成的 report 返回值,把一次成功评审变成失败。可改为 shutil.rmtree(..., ignore_errors=True) 或包一层 try/except OSError

总结

整体实现质量较高:Filter 的脚本/路径/网络/环境/预算治理、多层脱敏、原子写报告、去重与置信度分桶逻辑自洽,且本地路径有较完整的用例覆盖。未发现必须修复的 Critical 问题;主要风险在于生产默认的 container 运行时在 CI 中未被覆盖,以及输出预算 CLI 参数存在反直觉的拦截行为。

测试建议

  • 在 CI 中增加一条以 RUN_CODE_REVIEW_CONTAINER_TEST=1 触发 test_container_runtime_end_to_end 的任务,覆盖生产默认的 container 路径(mount、输出取回、auto_remove 清理)。
  • 补充一条用例:调大 --max-output-bytes 同时同步调大 --max-policy-output-bytes,验证大输出场景不会被 output_budget 误拦截并能正常落库。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

基于 Skills + 沙箱 + 数据库存储构建自动代码评审 Agent

2 participants