Skip to content

examples: add skill-driven code review agent#233

Open
Enjoykkk wants to merge 2 commits into
trpc-group:mainfrom
Enjoykkk:feat/code-review-agent
Open

examples: add skill-driven code review agent#233
Enjoykkk wants to merge 2 commits into
trpc-group:mainfrom
Enjoykkk:feat/code-review-agent

Conversation

@Enjoykkk

Copy link
Copy Markdown

Summary

新增 examples/skills_code_review_agent 自动代码评审 Agent 示例。

该示例支持输入 unified diff、Git 工作区变更或文件列表;通过加载代码评审 Skill、在隔离 Workspace 中执行受控检查,生成结构化 findings、JSON/Markdown 报告和可按 task id 回读的审计记录。

Related Issue

Fixes #92

PR Type

type/feature

Changes

  • 新增基于 LlmAgentSkillToolSetRunner 的代码评审流程。
  • 新增 unified diff、仓库变更和文件列表输入解析,以及 hunk 与候选行提取。
  • 新增敏感信息脱敏、确定性评审规则、finding 校验、去重和低置信度人工复核。
  • 新增 Docker、local 开发 fallback 和 Cube Runtime 入口。
  • 新增命令白名单、网络/依赖安装拦截、禁止路径、超时预算和 stdin 治理。
  • 新增 SQLite 审计存储,保存任务、findings、Filter 决策、Skill 运行、模型调用、监控指标和最终报告。
  • 新增按 task id 划分的 review_report.jsonreview_report.md
  • 新增 --dry-run--fake-model 与真实 OpenAI 兼容模型三种运行模式。
  • 新增公开 fixture,覆盖无问题 diff、敏感信息、异步资源、数据库事务、测试缺失、去重、沙箱失败和脱敏。

Skill Attribution

skills/code-review/awesome-skills/code-review-skill 引入,并保留上游许可证。

Validation

python -m pytest tests/examples/test_code_review_agent.py -q
python -m compileall -q examples/skills_code_review_agent
git diff --check

已手动验证 Docker Runtime 下的 --diff-file--repo-path 输入路径。

Add a code review example that loads the code-review Skill, stages review
inputs in supported workspace runtimes, applies execution governance, and
persists structured reports and audit metadata.

The example supports dry-run, FakeModel, and OpenAI-compatible model paths,
with Docker/Cube runtime configuration, fixtures, and Chinese documentation.

RELEASE NOTES: Added a skill-driven code review Agent example with sandboxed
review execution and structured reports.
@Enjoykkk

Copy link
Copy Markdown
Author

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

@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@86781c7). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             main        #233   +/-   ##
==========================================
  Coverage        ?   87.84906%           
==========================================
  Files           ?         482           
  Lines           ?       45157           
  Branches        ?           0           
==========================================
  Hits            ?       39670           
  Misses          ?        5487           
  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.

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

发现的问题

⚠️ Warning

  • examples/skills_code_review_agent/agent/tools.py:177-183:diff 引用路径可穿越仓库根目录,导致宿主机任意文件被 stage 进沙箱

    • parse_review_input--repo-path 模式下用 (root / line.file).resolve() 解析 diff 中 +++ b/<path> 指向的文件,line.file 来自不可信 diff,可写成 ../../etc/secrets.pyresolve() 后落到 root 之外,只要该文件存在就会被加入 sources 并经 _stage 拷进沙箱(仅用 source.name 作为 dst)。当前 analyze_pr 不读取这些源文件,但 README 明确 ruff/pytest 即将作为 action 暴露,届时会变成可读的宿主文件泄露。修复:仅当 candidate.is_relative_to(root) 为真且未含 .. 跳出时才纳入 sources,否则跳过。
    ...
    candidate = (root / line.file).resolve()
    if candidate.is_file():
        sources.add(candidate)
    ...
  • examples/skills_code_review_agent/agent/tools.py:38-39:宿主机绝对路径通过 workspace_inputs 写入模型输入与审计,违反 README/instruction 声称的“模型不获取宿主机绝对路径”

    • _stage 生成 host://{absolute_path} 并经 model_dump() 进入 result["workspace_inputs"]run_review.py 又把整个 review_input 作为 payload 文本发给 Runner;before_model_audit 还把该 request 原样存入 code_review_model_runs 审计(redact 只脱敏密钥不脱敏路径),最终落盘到 reviews.sqlite 与报告。对不可信 PR 场景,等于把宿主机目录结构泄露给模型和审计读者。修复:发送给模型/审计前剥离 src(仅保留 dst),执行器需要的 host 映射从 ctx.metadata 读取(_review_action_tool 已是这样取的)。
  • examples/skills_code_review_agent/.env:9:将 .env 作为受版本控制文件提交,易引发真实密钥误提交

    • 仓库根 .gitignore 未忽略 .env,该文件被追踪;用户按 README “编辑仓库提供的 .env 模板”填入真实 OPENAI_API_KEY 后,git status/git add 极易把密钥带入提交。修复:改名为 .env.example 并在 .gitignore 中忽略 .env,运行时仍 load_dotenv(".env")

💡 Suggestion

  • examples/skills_code_review_agent/agent/policy.py:17ReviewPolicyEngine.decidecommand.split() 做白名单与 -c 检测,token 化过于朴素(如 python -c"..." 会让 command[:2] != ["python","-c"] 从而绕过动态代码判定)。当前因模型只能触发固定 analyze_pr action 而不可利用,但作为防御纵深建议用 shlex.split 并对 shell 元字符单独拦截,避免后续放开 action 时被绕过。

  • examples/skills_code_review_agent/agent/storage.py:31-32with self._connect() as db: 只管理事务、不关闭连接,连接释放依赖 GC;长会话多次调用 save_native/get_task 会堆积句柄。建议在 _connect 返回后显式 try/finally db.close(),或改用上下文管理器封装。

总结

整体为示例性质实现,核心编排与脱敏/审计链路设计合理,当前不存在可直接利用的 Critical 漏洞。存在 3 处建议修复的 Warning:diff 路径穿越导致宿主文件被 stage、宿主机绝对路径泄露给模型与审计、.env 作为受追踪文件带来的密钥误提交风险。

测试建议

  • 补充 parse_review_input 的路径穿越用例:--repo-path 下传入含 +++ b/../../<外部文件> 的 diff,断言该文件未出现在 workspace_inputs
  • 补充断言:发给模型的 payload 文本与持久化的 model_runs 审计中不含 host:// 绝对路径。

@Enjoykkk Enjoykkk closed this Jul 27, 2026
@Enjoykkk Enjoykkk reopened this Jul 27, 2026
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

发现的问题

🚨 Critical

  • examples/skills_code_review_agent/agent/tools.py:275-276save_review_report 未校验模型传入的 task_idoutput_dir,存在路径穿越/任意文件写入风险
    • 该函数作为 FunctionTool 暴露给模型(agent/tools.py:454),task_idoutput_dir 均由模型/不可信 diff 上下文提供。task_root = root / task_id 后直接 task_root.mkdir(parents=True, exist_ok=True) 并写 review_report.json/.md,若 task_id../../output_dir 指向任意路径,可在宿主机任意目录创建目录并写入文件,SQLite 主键也会被污染。建议对 task_id 做白名单字符校验(如 ^[A-Za-z0-9._-]+$ 且禁止 ..),并强制 task_root.resolve().is_relative_to(root.resolve())
    ...
    root, task_root = Path(output_dir or Path(__file__).parents[1] / "review-output"), None
    task_root = root / task_id   # task_id 来自模型,未校验
    task_root.mkdir(parents=True, exist_ok=True)
    ...

⚠️ Warning

  • examples/skills_code_review_agent/.env:1-20:真实环境文件 .env 被纳入版本库跟踪

    • 该文件为 load_dotenv 实际读取的配置文件,当前虽为占位符,但被 git ls-files 确认已跟踪。后续开发者填入真实 OPENAI_API_KEY/E2B_API_KEY 后极易被误提交泄露。建议改为提交 .env.example,将 .env 加入 .gitignore(当前 .gitignore 未忽略它)。
  • examples/skills_code_review_agent/agent/storage.py:87,99get_task 读取的 sandbox_runs 表从未被写入,审计字段恒为空

    • save_native 只写 skill_runs,不写 sandbox_runs,因此 get_task 返回的 sandbox_runs 永远是 [],与 metrics.sandbox_run_count(取自 skill_runs 长度)语义不一致,会误导下游审计/查询。建议删除该表与查询,或与 skill_runs 统一。
  • examples/skills_code_review_agent/agent/tools.py:179parse_review_inputrepo_path 直读仓库时,若未传 staging_dir 则不会 stage 生成的 diff

    • 仅当 root and staging_dir 同时为真才写出 input.diff;当该函数被未来调用方以 repo_path 但无 staging_dir 调用时,沙箱内不会有 work/inputs/input.diff,导致 analyze_pr--diff-file ../../work/inputs/input.diff)必然失败。当前 CLI 路径总会传 staging_dir,但函数为公共入口,建议在 root 存在时强制要求 staging_dir 或自动回退到 files/diff 模式并明确报错。
  • examples/skills_code_review_agent/agent/policy.py:30-c 动态代码检测可被参数拼接绕过

    • 检测条件为 token == "-c" or token.startswith("-c"),对 python 直接执行脚本文件无拦截,但若模型传入 python+通过环境变量或 PYTHONSTARTUP/PYTHONPATH 间接执行代码则不在拦截范围;同时 startswith("-c") 会误拦 -cache 类不存在的参数。建议改为精确匹配 -c/-c 后跟代码(如 token == "-c" 或正则 ^-c$|^-c.+)并补充对 python 执行非工作目录脚本的审计。当前 stdin 已被 filter 拦截,风险可控但建议收紧。

💡 Suggestion

  • examples/skills_code_review_agent/agent/filter.py:55-58ruff 仅 diff 场景的拦截判断用 not any(path != ...) 表达“全部为 diff”,可读性差且与 dst 语义耦合,建议封装为显式变量(如 has_staged_source = any(...))以降低后续维护误改风险。

总结

整体安全边界(脱敏、host 路径隔离、命令白名单、stdin 拦截、路径穿越 staging 校验)设计较完整,但 save_review_report 对模型传入的 task_id/output_dir 缺少校验,存在任意文件写入的 Critical 风险,必须修复;.env 入库与 sandbox_runs 死表为需关注的 Warning。

测试建议

  • 增加对抗性测试:模型传入 task_id="../../evil"output_dir 指向仓库外路径时,断言 save_review_report 拒绝写入并报错。
  • 增加测试覆盖 parse_review_input(repo_path=..., staging_dir="") 的行为契约(报错或安全回退),避免未来调用方误用导致沙箱缺 diff。

continue
seen.add(key)
(human if item["confidence"] < 0.7 else accepted).append(item)
root, task_root = Path(output_dir or Path(__file__).parents[1] / "review-output"), None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save_review_report 未校验 task_id/output_dir 致路径穿越

该函数作为 FunctionTool 暴露给模型,task_id 与 output_dir 由不可信上下文提供,未做校验即拼接 task_root 并 mkdir/写文件,存在任意文件写入风险。建议对 task_id 做白名单字符校验(如 ^[A-Za-z0-9._-]+$ 且禁止 ..),并强制 task_root.resolve().is_relative_to(root.resolve())。

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