Skip to content

example: add skills-based code review agent - #252

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

example: add skills-based code review agent#252
2021210507 wants to merge 2 commits into
trpc-group:mainfrom
2021210507:feat/skills-code-review-agent

Conversation

@2021210507

Copy link
Copy Markdown

概述

实现一个端到端、可验证的自动代码评审 Agent 原型,交付目录为:

examples/skills_code_review_agent/

输入支持 unified diff / PR patch、指定文件列表、Git 工作区变更和内置 fixture。系统通过 code-review Skill 加载受控规则脚本,经 Filter 前置治理后在 SDK workspace sandbox 中执行检查;随后将问题按严重级别、文件、行号、证据和修复建议结构化输出,并把任务、Filter 决策、沙箱运行、finding、报告与指标写入 SQLite。

核心取舍是“确定性规则负责检出,LLM 仅做受限文本增强”:

  • 正则 + Python AST 规则保证 fake model / dry-run 下仍可复现、可测试;
  • LLM 只能增强 summary、recommendation 和人工复核提示;
  • LLM 不得增删 finding,也不得修改 identity、severity、confidence、bucket 或去重结果;
  • 无 API Key 时可使用 dry-run / fake model 完整验证解析、Filter、沙箱和落库链路。

架构

diff / files / Git workspace / fixture
  → 输入校验与受控 staging
  → code-review Skill:diff 解析 + 确定性规则
  → Filter 前置治理(DENY / NEEDS_HUMAN_REVIEW 不进沙箱)
  → SDK workspace sandbox
       - Container:生产默认,验证 network_mode=none
       - Local:仅显式开发 fallback
       - Cube:无可验证无出口网络证明时默认拒绝
  → finding 去重与四桶路由
       - dedup key: (file, line, category)
       - findings / needs_human_review / suppressed / warnings
  → 三层脱敏与最终出口扫描
  → canonical review_report.json
       ├── review_report.md
       ├── SQLite 五张 cr_* 业务表
       └── metrics / telemetry / audit summary

Agent 入口实际复用 SDK LlmAgent + SkillToolSet

user-query
  → skill_load("code-review")
  → 受控 skill_run(request_id)
  → ReviewPipeline.run()

CLI、Agent、dry-run 和测试共用同一个 ReviewPipeline,不复制规则或报告逻辑。

核心能力

  • code-review Skill:包含 SKILL.md、6 类规则文档、manifest 和隔离脚本。
  • 规则覆盖:安全风险、敏感信息、异步错误、资源泄漏、数据库生命周期、测试缺失。
  • 四类输入--diff-file--repo-path--files--fixture,互斥校验。
  • 结构化 finding:severity、category、file、line、title、evidence、recommendation、confidence、source。
  • SQLite 持久化cr_review_taskcr_sandbox_runcr_filter_eventcr_findingcr_report;支持按 task id 查询完整 bundle。
  • 安全边界:manifest allowlist、超时、单次/总输出上限、环境变量白名单、网络拒绝、失败即数据、无宿主回退。
  • 脱敏闭环:检测与脱敏共享规则源,原始 diff、代码、环境变量、明文凭据不会进入 LLM、日志、Telemetry、报告或数据库。
  • 报告:canonical JSON schema 校验后原子写入,再由同一对象确定性渲染 Markdown。

验收对照(issue #92

# 验收项 状态 当前证据
1 8 条公开 diff 样本可运行并生成报告 8 个 simple fixture 分别验证 JSON、Markdown 与 SQLite;另提供 8 个 complex 工程样例。
2 高危检出率 ≥80%,误报率 ≤15% ✅* 固定公开代理语料:Recall 100%(16/16),finding 级误报 0%(0 FP);官方隐藏样本仍待官方验收。
3 DB 完整记录并支持按 task 查询 SQLite 五表记录 task、sandbox run、Filter event、finding、report;CLI 支持 show / list / init-db。
4 沙箱超时、输出限制、失败不崩溃 timeout、非零退出和输出截断均记录为 sandbox run + warning;可出报告时为 completed_with_warnings。
5 脱敏 ≥95%,无明文 公开代理语料脱敏检出 100%(48/48),plaintext_hits=0。
6 dry-run / fake model ≤2 分钟 8 条 simple fixture 分别独立运行 fake + local Agent,最大观测值 27.312 s。
7 Filter deny / review 不进沙箱 Filter 前置短路;测试断言 sandbox run 数为 0,并持久化脱敏拒绝事件。
8 报告包含完整摘要和修复建议 JSON schema、Markdown 和 sample output 覆盖 findings、严重级别、人工复核、Filter、sandbox、metrics、结论和 recommendation。

使用方法

Windows PowerShell:

$py = ".\.venv\Scripts\python.exe"
& $py -m pip install -e ".[dev]"
& $py -m pip install -r examples/skills_code_review_agent/requirements.txt
& $py examples/skills_code_review_agent/run_agent.py review `
  --fixture 02_security_simple `
  --sandbox local `
  --dry-run `
  --output-dir out/review_quickstart `
  --db-url sqlite+pysqlite:///out/review_quickstart/review.db

Linux/macOS Bash:

py=".venv/bin/python"
"$py" -m pip install -e ".[dev]"
"$py" -m pip install -r examples/skills_code_review_agent/requirements.txt
"$py" examples/skills_code_review_agent/run_agent.py review \
  --fixture 02_security_simple \
  --sandbox local \
  --dry-run \
  --output-dir out/review_quickstart \
  --db-url sqlite+pysqlite:///out/review_quickstart/review.db

真实 Agent + Skill 路径:

& $py examples/skills_code_review_agent/run_agent.py user-query `
  "请使用 code-review Skill 审查安全风险样例" `
  --fixture 02_security_simple `
  --sandbox container `
  --model-mode real `
  --trace `
  --output-dir out/review_real `
  --db-url sqlite+pysqlite:///out/review_real/review.db

真实模型模式需要在 examples/skills_code_review_agent/.env 配置:

TRPC_AGENT_API_KEY=<key>
TRPC_AGENT_BASE_URL=<base-url>
TRPC_AGENT_MODEL_NAME=<model-name>

验证命令

$py = ".\.venv\Scripts\python.exe"
& $py examples/skills_code_review_agent/evaluate.py --sandbox local --output-dir out/eval_local
& $py -m pytest examples/skills_code_review_agent/tests -q -m "not container and not real_llm" -p no:cacheprovider
& $py -m flake8 examples/skills_code_review_agent

可选集成:

& $py -m pytest examples/skills_code_review_agent/tests/integration -q -m container
& $py -m pytest examples/skills_code_review_agent/tests/integration -q -m real_llm

文件结构

examples/skills_code_review_agent/
├── agent/                 # LlmAgent、SkillToolSet、受控工具入口
├── codereview/            # inputs、pipeline、governance、sandbox、store、report、metrics
├── skills/code-review/    # SKILL.md、规则文档、manifest、隔离脚本与规则实现
├── schemas/               # canonical JSON report schema
├── sample_output/         # JSON / Markdown 示例报告
├── tests/                 # unit / integration / e2e / fixtures
├── run_agent.py           # review / user-query / show / list / init-db
├── evaluate.py            # 离线公开代理评测
├── requirements.txt       # 示例额外依赖
├── README.md
├── OPERATIONS.md
├── DESIGN.md
└── DEV_SPEC.md

Windows host → Linux Container 的 SDK 路径兼容修复由 #251 单独处理,避免混入本示例 PR。

Closes #92

@2021210507

Copy link
Copy Markdown
Author

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

@2021210507

Copy link
Copy Markdown
Author

recheck

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

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

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

@2021210507

Copy link
Copy Markdown
Author

@helloopenworld 求证下报错的原因

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

发现的问题

🚨 Critical

(未发现必须修复的阻塞性问题。manifest 中各脚本 sha256 与磁盘文件一致;governance/sandbox 的 fail-closed、路径穿越、shell 注入、网络白名单与三层脱敏链路在 diff 范围内自洽。)

⚠️ Warning

  • examples/skills_code_review_agent/codereview/redaction.py:20-21(及 codereview/inputs.py:92-94):模块导入即向 sys.path[0] 插入 Skill 的 scripts 目录,全局注册一个名为 lib 的顶层包

    • 只要 import codereview.redaction(pipeline/agent 等均间接导入),就会把 lib 安装到解释器最前搜索位;若宿主进程或同会话其它代码也依赖顶层 lib 包,会被静默覆盖为 Skill 内实现,造成难以定位的导入分叉。建议改为命名空间隔离(如 importlib.util 按绝对路径加载为专属模块名),避免占用通用 lib 顶层名。
  • examples/skills_code_review_agent/evaluate.py:77-92_sanitized_environment 将子进程 PATH 仅设为 sys.executable 父目录,使 --sandbox container 评测路径必然失败

    • container 分支调用 run_agent.py review --sandbox container,其内部 _container_available()shutil.which("docker") 搜索 PATH;此时 PATH 不含系统目录,docker 找不到 → 返回 container_runtime_unavailable → 子进程非零退出 → _run_skill_checkstrusted_skill_execution_failed。local 路径不经 run_agent 故不受影响,但文档声明的 container 评测选项不可用。建议在 sanitized PATH 中追加系统 PATH(或显式解析 docker 绝对路径后注入)。
    ...
    environment["PATH"] = str(Path(sys.executable).parent)
    ...

💡 Suggestion

总结

整体安全边界与确定性检测链路在 diff 范围内自洽且无明显阻塞问题;主要风险集中在两处兼容性/可运行性细节——redaction.py 全局 sys.path 注入顶层 lib 的潜在导入分叉,以及 evaluate.py 在 container 沙箱下因 PATH 被裁剪导致该评测路径不可用。均非核心功能失败,建议修复但不阻塞合入。

测试建议

  • evaluate.py --sandbox container 增加一条断言:在 docker 可用环境下子进程能成功解析并产出报告(或显式校验 PATH 透传后 shutil.which("docker") 命中),覆盖当前被裁剪 PATH 破坏的容器评测路径。
  • 增加一条隔离导入测试:在已 import codereview.redaction 的会话中加载一个同名顶层 lib 干扰模块,断言 skills lib 不会覆盖宿主预期 lib(验证 sys.path 注入边界)。

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

很好——这证实了 cube 的治理(governance)总是被拒绝(已测试),并且 CLI 公开了 cube 但未连接 factory。所以 --sandbox cube 总是因为 cube_runtime_unavailable 而失败。已确认的问题。

现在让我完成审查。让我写下最终的审查结果。

发现的问题

🚨 Critical

(无)

⚠️ Warning

  • examples/skills_code_review_agent/run_agent.py:490--sandbox cube 选项对外暴露但实际不可用
    • CLI 将 cube 作为 --sandbox 的合法取值,但 build_review_pipeline 调用 create_sandbox_runtime(args.sandbox, ...) 时未传入 cube_runtime_factory(默认 None),sandbox.py:454-455 会直接抛 SandboxConfigurationError("cube_runtime_unavailable"),被 run_agent.py:210-211 收敛为 CliError("sandbox_configuration_invalid") 并以退出码 2 失败。即任何 --sandbox cube 调用必然失败;同时 governance.py 中 cube 路径即便有工厂也会因 network_policy_verified=False 被 DENY。建议从 choices 移除 cube(或在文档/帮助中标注不可用),避免误导调用方与 CI 脚本。

💡 Suggestion

  • examples/skills_code_review_agent/codereview/sandbox.py:870-935change_set_payload 重建 diff 时丢弃 \ No newline at end of file 标记
    • 重建循环只处理 +/-/ 行,遇到 "No newline" 标记会进入 else 分支抛 sandbox_change_set_hunk_invalid(若标记出现在非预期位置)或被跳过。当前因 full_files/file_metadatarestore_change_set_context 恢复真实上下文,丢失标记不影响最终行号与规则结果,但路径较脆弱;可在重建时识别并跳过该标记以提升鲁棒性。

总结

整体未发现必须修复的 Critical 问题;沙箱、脱敏、治理、预算与报告链路均有较强的失败关闭与二次校验。唯一明确问题是 --sandbox cube 选项被暴露但恒不可用(Warning),建议从 CLI choices 移除或显式标注。

测试建议

暂无额外测试建议;现有单测/集成/e2e 已覆盖脱敏出口、预算限额、治理拒绝与报告原子写入等高风险路径。如采纳 cube 移除建议,可补一条 --sandbox cube 必定失败的 CLI 约束测试。

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

我现在已经有了全面且深入的了解。让我来整理一下审查意见。

发现的问题

🚨 Critical

未发现 Critical 级别问题。

⚠️ Warning

  • examples/skills_code_review_agent/codereview/sandbox.py:294-303execute 在检测到已存在事件循环时静默返回 sandbox_event_loop_unsupported 错误而非抛出异常。

    • 当前 Agent 流程通过 asyncio.to_thread 在独立线程运行 pipeline.run,因此不会命中该分支;但若未来调用方在 async 上下文中直接调用 pipeline.run,评审会被静默降级为 error 状态而非快速失败,可能掩盖集成错误。建议将该情形收敛为明确异常或至少在 pipeline 层记为可区分的失败,避免“沙箱未执行却看似正常完成”。
  • examples/skills_code_review_agent/codereview/sandbox.py:910-923change_set_payload 重建 hunk 时用 old_line/new_line 游标顺序匹配 deleted→added→context,依赖输入 ChangeSet 已由 parse_unified_diff/build_snapshot_change_set 保证结构合法。

    • 该重建逻辑对“同一行号同时存在 context 与 added/deleted 的非典型 hunk”是脆弱的:一旦未来 ChangeSet 来源扩展或 hunk 字典被外部构造出现 old/new 行号错位,循环可能输出错误行序或落入 ValueError。建议对重建结果在沙箱侧 parse_unified_diff 回程校验行数一致性,或直接随载荷传递已验证的 hunk 结构而非二次重建 diff 文本。
  • examples/skills_code_review_agent/codereview/inputs.py:202-218_run_git 把可执行体 resolved_executable 相对于 repo 解析,仅当 git 落在仓库内才拒绝,未校验 shutil.which("git") 返回路径本身的可信来源。

    • 真实风险有限(subprocess.run 以 argv 形式调用、check=True、15s 超时、stderr 不外泄),但 PATH 中被注入的恶意 git 仍会被执行。属于不可信输入执行边界的纵深防御缺口,建议对 git 可执行路径做白名单或固定来源校验。

💡 Suggestion

  • examples/skills_code_review_agent/skills/code-review/scripts/lib/secret_rules.py:110-114discord_token 模式 \b[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_-]{20,}\b 过于宽泛,会命中常见版本号或三段式标识。当前因评测语料的 benign 用例均走 token= 赋值路径而不触发该模式,暂不影响门禁;但作为通用脱敏源它会带来误报噪声,建议收紧为 Discord token 的真实前缀/字符集特征。

总结

整体实现质量很高:输入边界、沙箱治理、manifest 完整性、脱敏与 fail-closed 错误收敛均有充分设计并有测试覆盖(含 manifest 哈希一致性、网络策略、预算、密钥脱敏等关键路径)。未发现必须修复的阻塞性问题;上述 Warning 均为纵深防御或未来扩展鲁棒性建议,不影响当前正确性。

测试建议

  • 建议补充一条用例:在已运行事件循环的上下文中调用 SdkSkillSandbox.execute,断言其行为符合预期(明确失败而非静默 error),以固化 sandbox_event_loop_unsupported 的契约。
  • 建议补充 change_set_payload 对非典型 hunk(old/new 行号错位)输入的防御性回归,确保二次重建不会产生静默错误行序。

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

我现在已经有了足够的验证结论。正在汇总审查结果。

发现的问题

🚨 Critical

  • examples/skills_code_review_agent/skills/code-review/scripts/lib/diff_parser.py:262-263(及 rules_ast.py:435-436):ast.parse 仅捕获 SyntaxError,null 字节会抛 ValueError 导致沙箱入口崩溃
    • 解析不可信 diff 内容时,若 .py 文件文本含 \x00ast.parse 抛出 ValueError(非 SyntaxError 子类),当前 except SyntaxError 无法捕获,异常会穿过 _analysis_mode/_ast_candidates 直接中断 parse_unified_diff 和整个沙箱脚本运行。两处需同时改为 except (SyntaxError, ValueError),失败时回退到 diff_heuristic 模式。
      try:
          ast.parse(full_text)
      except SyntaxError:
          return "diff_heuristic", f"ast_parse_failed:{path}"

⚠️ Warning

  • examples/skills_code_review_agent/codereview/pipeline.py:481-484:parse/governance 阶段异常被笼统标记为 stage="sandbox"

    • 外层 try 同时覆盖 input 解析、governance.decide 和 sandbox 执行,但 except Exception 一律以 stage="sandbox" 记录告警并继续;governance 链抛出的非致命异常被吞掉并错误归因为沙箱问题,违背 fail-closed 意图且误导排障。建议按阶段拆分 try 或在 handler 中区分异常来源。
  • examples/skills_code_review_agent/codereview/pipeline.py:565-567:LLM 链路 error_type 由报告状态反推,误报 llm_enhancement_failed

    • status="completed_with_warnings" 可在 LLM 之前(line 511)就已设置;当先前已有告警而 LLM 增强成功时,telemetry span 仍被打上 error_type="llm_enhancement_failed",污染监控。建议用局部布尔(如 llm_failed)记录 except 分支是否实际触发,而非从 report["status"] 反推。
  • examples/skills_code_review_agent/codereview/sandbox.py:379(及 build_run_spec 489-507):local 运行态以宿主 sys.executable 与宿主 site-packages 执行 Skill 脚本

    • SanitizedLocalProgramRunner 只过滤环境变量,use_workspace_root=True 时脚本仍在宿主解释器和 sys.path 下运行,可 import 宿主已装包(含内嵌凭据的云 SDK/DB 驱动),弱化沙箱隔离目标。local 虽为显式 fallback,但应至少在文档/告警中明确该隔离限制,或额外约束 PYTHONPATH/可用包。
  • examples/skills_code_review_agent/skills/code-review/scripts/lib/rule_engine.py:66:三引号状态机对单行多个三引号串处理错误

    • advance_triple_quote_state 在一行含 '''a''' x='''b 时返回 (None, True),把后续行误判为代码而非字符串内容,导致漏报(字符串内容被当代码扫)与误报。属于真实解析缺陷,建议按首尾三引号正确配对推进状态。

💡 Suggestion

  • examples/skills_code_review_agent/codereview/sandbox.py:778:entrypoint 的 sha256 仅校验长度(len != 64)未校验十六进制,与 parse_integrity_files 对 integrity 文件 hash 使用 _SHA256_PATTERN 不一致;虽因后续比对不可利用,但为一致性建议同样 fullmatch

总结

整体风险偏低:无凭证泄露/命令注入/SQL 注入(subprocess 全用 argv 数组、ORM 参数化、env 白名单过滤到位)。最需修复的是 ast.parse 未捕获 ValueError 导致不可信 diff 可使沙箱入口崩溃(Critical),以及 pipeline 异常归因与 telemetry 误标两类 Warning。不存在阻断合并的硬性安全漏洞。

测试建议

  • 补充含 null 字节的 .py diff fixture,断言 parse_unified_diff 不抛未捕获异常并回退到 diff_heuristic
  • 补充 pipeline 用例:先产生 parse/governance 告警后再走 LLM 成功路径,断言 emit_span("llm")error_typeNone

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

我已完成审查,正在整理最终结论。

发现的问题

🚨 Critical

未发现 Critical 问题。沙箱隔离、凭据脱敏、manifest 完整性校验(sha256 与实际文件一致)、路径穿越/符号链接防护、fail-closed 治理、原子写入和预算预检均经核对无明确缺陷。

⚠️ Warning

  • examples/skills_code_review_agent/tests/integration/test_inputs.py:167-172:符号链接/junction 测试在 Linux 上必然失败
    • 该测试无条件调用 subprocess.run(["cmd", "/c", "mklink", "/J", ...], check=True) 创建 Windows 目录联接,在 Linux/macOS 上 cmd 不存在会抛 FileNotFoundError,导致整条用例(含 ../outside.py、绝对路径、junction/secret.py 三个关键穿越/链接拒绝场景)无法运行。建议用 sys.platform/shutil.which("cmd") 守卫,Windows 走 mklink、POSIX 走 os.symlink,否则该安全断言在主流平台形同虚设。需注意:仓库根 CI 的 testpaths=["tests"] 不收集 examples/ 下测试,故不影响主干 CI,但会阻塞在本地运行示例测试套件的开发者。
    ...
    subprocess.run(
        ["cmd", "/c", "mklink", "/J", str(junction), str(outside_dir)],
        check=True,
        capture_output=True,
        text=True,
    )
    ...

💡 Suggestion

  • examples/skills_code_review_agent/code_review/pipeline.py:451candidate_count=len(_safe_candidates(raw_sandbox_result)) 仅为统计候选数,却对全部 finding 重复执行了一次 redact_data(dict(finding))_safe_candidates 第 465 行已对同一结果做过同样脱敏)。可改为直接 len(raw_sandbox_result.get("findings") or [])(在确认是 list 时),避免一次无用的递归脱敏开销;不影响正确性。

总结

整体实现质量高、防御性强,核心安全与正确性路径(沙箱网络隔离、凭据脱敏闭包、manifest sha256 完整性、路径穿越与符号链接防护、fail-closed 治理)均无阻塞问题。唯一明确问题是示例测试中一条 Windows 专用用例在 Linux 上不可运行,影响示例测试套件的可移植性,但不进入主干 CI。

测试建议

  • test_files_reject_traversal_absolute_and_symlink_before_reading 增加平台分支(POSIX 用 os.symlink),使其在 Linux CI/本地也能验证链接拒绝路径。
  • 暂无其他额外测试建议。

@2021210507

Copy link
Copy Markdown
Author

recheck

1 similar comment
@2021210507

Copy link
Copy Markdown
Author

recheck

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

基于我对 PR diff 及相关上下文的全面审查,以下是我的代码审查结果。

发现的问题

🚨 Critical

  • examples/skills_code_review_agent/tests/e2e/test_release_docs.py:155-168test_release_docs_explicitly_mark_cube_as_unavailable 断言 --sandbox cube --dry-run 出现在 OPERATIONS.md 中,但 run_agent.py:490 仍把 cube 列为 --sandbox 合法选项,且 create_sandbox_runtimecube_runtime_factory is None 时抛 cube_runtime_unavailable。当用户/CI 执行 --sandbox cube --dry-run 时会直接以运行时错误退出,而非 CLI 校验期拒绝;这与“CLI 尚未注入 cube_runtime_factory”的文档承诺不一致,属于可被触发的核心入口失败路径。建议在 argparse 层或 build_review_pipeline 早期显式拒绝 cube,或确认断言对应的文档/CLI 契约一致。

⚠️ Warning

  • examples/skills_code_review_agent/code_review/sandbox.py:294-322execute/cleanup 通过 asyncio.get_running_loop() 检测并返回 _sandbox_error("sandbox_event_loop_unsupported")/抛 RuntimeError,但该分支无任何测试覆盖。Agent 入口(agent/agent.py:190 asyncio.run)与 pipeline 同步边界混合时,一旦在已有事件循环线程内调用 pipeline 会静默返回 error 状态而非可诊断失败。建议补充“在运行中事件循环内调用 sandbox.execute/cleanup 返回结构化错误”的测试以锁定该契约。

  • examples/skills_code_review_agent/code_review/store/models.py:19-22:从内部模块 trpc_agent_sdk.storage._sql_common 导入 DynamicJSON/PreciseTimestamp,而 SDK 自身代码(trpc_agent_sdk/memory/_sql_memory_service.py:38sessions/_sql_session_service.py:58)一律通过公共出口 from trpc_agent_sdk.storage import ... 导入同一批符号(__init__.py:59-62 已 re-export)。依赖下划线私有路径存在被重构破坏的兼容性风险,应改用公共导入。

  • examples/skills_code_review_agent/code_review/llm_enhancer.py:281-282_merge_text_only 在模型篡改 finding 身份时 raise ValueError("llm_attempted_to_mutate_finding_identity"),但该高风险防护分支无测试覆盖(tests/integration/test_llm_enhancer.py 仅有 fake/real 正常路径)。该异常会经 enhance 冒泡到 pipeline 被 except Exception 收敛为 llm_enhancement_failed warning,缺少验证“模型尝试改 severity/file/line 时被拒绝”的回归测试。

  • examples/skills_code_review_agent/code_review/governance.py:342manifest.json:25,52_argument_reason 读取 template.get("additional_properties")(下划线风格),而标准 JSON Schema 字段为 additionalProperties。当前 manifest 与 test_skill_scripts.py:157 用下划线形式自洽,但这偏离 JSON Schema 约定,后续若引入标准 schema 校验或第三方工具会静默失效(additionalProperties 未设时默认允许,当前靠下划线字段 + 显式 name not in properties 兜底才安全)。建议统一为 additionalProperties 或在文档注明这是私有契约。

  • examples/skills_code_review_agent/evaluate.py:115-141_run_skill_checkssandbox == "local" 时直接以 [sys.executable, str(RUN_CHECKS_PATH)] 运行脚本,绕过了 run_agent.py 的输入校验、治理与脱敏管线;该路径写入 work/inputs/diff.json 后由 run_checks.py 直接读取。虽属离线评测,但这条“可信脚本直接执行”分支缺少对 fixture 载荷的二次校验,且与 container 分支(走完整 review CLI)的安全边界不一致,复用时易被误用为可信入口。

💡 Suggestion

  • examples/skills_code_review_agent/code_review/sandbox.py:280-292container_id 通过三层 getattrruntime.container.container.id)访问 SDK 私有结构,仅用于 INFO 诊断。该反射链对 SDK 内部结构变化极其敏感,建议封装为 SDK 公共诊断接口或降低对该属性形态的硬编码依赖(如 len==64 + hex 校验已做,但访问路径本身脆弱)。

总结

整体安全边界(沙箱网络 none、manifest 完整性、路径穿越、脱敏、预算预检)设计严谨且有较好测试覆盖。主要风险集中在 cube 入口契约不一致(Critical)以及若干高风险防护分支(事件循环拒绝、LLM 身份篡改拒绝)与私有导入路径缺少回归测试(Warning),建议合并前补齐。

测试建议

  • 补充 sandbox.execute/cleanup 在已有运行事件循环内调用时返回 sandbox_event_loop_unsupported/结构化错误的测试。
  • 补充 LlmEnhancer._merge_text_only 在模型返回篡改 severity/file/line 的响应时抛 llm_attempted_to_mutate_finding_identity 并被 pipeline 降级为 warning 的测试。
  • 补充 --sandbox cube 在 CLI 层被显式拒绝(而非运行时崩溃)的 e2e 测试,以与 test_release_docs.py 的断言对齐。

assert "review_fixture 02_security_simple agent" not in operations


def test_release_docs_explicitly_mark_cube_as_unavailable() -> 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.

cube 沙箱入口契约不一致

断言 OPERATIONS.md 包含 --sandbox cube --dry-run,但 run_agent.py 仍把 cube 列为合法选项,create_sandbox_runtime 在 cube_runtime_factory 为空时抛运行时错误而非 CLI 校验期拒绝,导致可触发的核心入口失败路径。建议在 argparse 层或 build_review_pipeline 早期显式拒绝 cube,或对齐文档与 CLI 契约。

@2021210507
2021210507 force-pushed the feat/skills-code-review-agent branch from da7eadc to 1886060 Compare July 29, 2026 11:54
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

发现的问题

🚨 Critical

  • tests/integration/test_inputs.py:167-172:Windows 专用 cmd /c mklink 在 Linux CI 必崩
    • 该测试用 subprocess.run(["cmd", ...], check=True) 创建 junction,在 Linux/macOS 上 cmd 不存在会抛 FileNotFoundError,整个测试用例失败而非跳过。缺少 pytest.mark.skipif(sys.platform != "win32") 平台门禁。建议改用 os.symlink(跨平台)或加平台跳过标记。

⚠️ Warning

  • code_review/pipeline.py:481-492:输入加载抛非 InputValidationError 时任务卡在 running 且治理失败被吞成 completed

    • _input_loader 若抛 OSError 等非 InputValidationError,会被第 481 行宽 except Exception 吞成 warning;change_set 仍为 None,第 491 行抛 PipelineFatalError未把任务状态从 running 更新为 failed,记录永久卡死。同一 except 还会吞掉 governance.decide() 的异常并继续生成零 finding 的 completed 报告,使本应 fail-closed 的治理层静默退化为“无问题”。建议:第 491 行抛错前 update_task(status="failed", ...),并把宽 except 收窄到仅包裹 sandbox 执行段,治理/输入异常应 fail-closed。
  • code_review/llm_enhancer.py:166code_review/llm_enhancer.py:217-221:增强链路无超时且 asyncio.run 缺运行中 loop 守卫

    • enhance() 直接 asyncio.run(...),若 pipeline 在已有事件循环上下文中被调用会抛 RuntimeError,被 pipeline.py:549 吞成 warning,增强静默失效。_run_agentasync for event in runner.run_async(...) 无外层超时,真实模型或多轮调用可无限挂起超过 review_deadline_seconds。建议加 get_running_loop() 守卫并用 asyncio.wait_for(..., timeout=review_deadline_seconds) 包裹迭代。
  • skills/code-review/scripts/lib/diff_parser.py:188skills/code-review/scripts/lib/diff_parser.py:510:单个畸形 hunk 中止整条 diff 解析

    • _parse_hunk 对任何非 +/-/ /\ 开头行或行数不匹配都抛 ValueError,而 parse_unified_diff 调用时无 try/except,一个文件的坏 hunk(如空行、计数不符)会导致整份 diff 解析返回零结果而非跳过坏文件。建议在 parse_unified_diff 内对单文件解析做隔离,记 parse_warning 后 continue。
  • skills/code-review/scripts/lib/rule_engine.py:196skills/code-review/scripts/lib/rules_ast.py:434-438:单条规则异常中止整次评审

    • RuleEngine.match 对每条 rule.match(change_set) 无 try/except;rules_ast._ast_candidates 仅捕获 SyntaxError,而 ast.parse 在含空字节或深层嵌套源码时可抛 ValueError/RecursionError,向上冒泡导致整次评审零 finding。建议逐规则 try/except 跳过失败规则,并把 AST 捕获扩展到 (SyntaxError, ValueError, RecursionError)
  • code_review/redaction.py:21skills/code-review/scripts/lib/secret_rules.py:脱敏仅靠静态正则白名单,非匹配格式密钥会原样落库

    • redact_text 只替换命中固定正则的子串;contains_plaintext_secret 用同套规则,无法发现漏脱敏。模型可控的 sandbox stdout/stderr 摘要、finding 证据被持久化到 DB 与报告,任何不在规则表中的高熵 token(hex/base64/AWS secret 配套串等)会原样外泄。建议对短高熵串叠加熵检测,或将 sandbox 输出摘要改为只持久化哈希/计数。
  • evaluate.py:598-600:评测运行时异常统一返回退出码 2,可能被 CI 当作基础设施错误绕过门禁

    • mainOSError/RuntimeError/ValueError/SubprocessError 时打印 evaluation_runtime_error 并返回 2,硬门禁 _hard_gates_pass 从不执行;若 CI 仅以退出码 1 视为门禁失败、2 视为可重试基础设施错误,则“阈值失败兜底”会绕过质量门禁。且该路径无测试覆盖。建议新增注入 _run_skill_checks 失败并断言退出码与 gate_evaluated:false 的测试,并确保 CI 对任意非零退出码均判失败。
  • code_review/sandbox.py:436-452code_review/sandbox.py:379:local runtime 在宿主以 sys.executable 运行且不强制网络隔离

    • runtime_type=="local"effective_network_mode=Nonenetwork_policy_verified=FalseSanitizedLocalProgramRunner 仅净化环境变量而不约束文件系统/网络;run_checks.py 以宿主用户在宿主目录执行并处理不可信 diff 输入。虽执行的是受控脚本而非任意 LLM 代码,但缺乏 FS/网络隔离构成纵深防御缺口。建议显式 local 模式仅允许 source_kind=fixture,或加 chroot/bwrap+nonet 隔离。
  • agent/agent.py:190(附近行):asyncio.run(self._run_sdk_agent(...)) 缺超时与轮次上限

    • fake/dry-run 因固定 3 步状态机安全,但 --model-mode realRunner 未设 max_turns/超时,模型反复发工具调用可无限挂起,直接 CLI 调用无 HARD_LIMIT_MS 保护。建议用 asyncio.wait_for(..., timeout=...) 包裹并设置最大轮次。
  • code_review/store/review_store.py:47-56code_review/store/review_store.py:192db_url 未校验后端与路径

    • _ensure_sqlite_parent--db-url 传入的路径 expanduser().resolve().parent.mkdir(parents=True) 可在任意位置建目录;create_engine 接受任意 SQLAlchemy URL(postgres/mysql 等),非 sqlite URL 静默放行。建议将 db_url 限制为 sqlite 且路径位于受控数据根目录内。

💡 Suggestion

总结

整体为新增示例工程,安全/隔离设计较完整但存在若干 fail-open 风险:最需关注的是 pipeline.py 把治理与输入异常吞成 completed 报告并可能留下卡死任务、脱敏仅靠正则白名单存在漏脱敏,以及 evaluate.py 退出码 2 路径可能绕过 CI 门禁;其中 test_inputs.py 的 Windows junction 测试在 Linux CI 上必然崩溃,建议优先修复。

测试建议

  • evaluate.py 退出码 2 路径补测试:注入 _run_skill_checks 失败,断言返回 2 且摘要标记门禁未评估。
  • pipeline.py 补测试:输入加载抛非 InputValidationError 时任务状态被置为 failed(而非卡 running),以及治理 decide 抛异常时整体 fail-closed 而非产出 completed。

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

该测试运行 run_checks.pycwd=workdir(工作空间根目录),其中 work/inputs/diff.json 是相对于根目录的。因此,该测试使用的是工作空间根目录的 cwd,这与 put_files 的写入位置相匹配。但沙箱 build_run_spec 在容器模式下将 cwd 设置为 staged_skill.workspace_skill_dir,而不是工作空间根目录。

这证实了不匹配的问题:在实际的沙箱容器执行中,run_checks.py 运行时的 cwd=workspace_skill_dir,但其相对输入路径 work/inputs/diff.json 是相对于工作空间根目录写入的。因此 run_checks.py 将无法找到其输入并失败。让我再次确认 workspace_skill_dir 是否确实在工作空间根目录下。

来自 stage_code_review_skill 的信息:result.workspace_skill_dir(由 SDK stager 返回)。通常类似于 skills/code-review。并且 workspace_entrypoint = f"{result.workspace_skill_dir}/scripts/{definition.entrypoint}"build_run_spec 中非根目录的 args = ["scripts/run_checks.py"]cwd = staged_skill.workspace_skill_dir。因此,python scripts/run_checks.py 是从 skills/code-review/ 目录运行的,而 _INPUT_PATH = Path("work")/"inputs"/"diff.json" 解析为 skills/code-review/work/inputs/diff.json —— 但文件位于工作空间根目录的 work/inputs/diff.json

这是一个真实的正确性问题。但是 —— 等等,它真的到达了 run_checks.pymain() 函数吗?run_checks.py 调用了 _load_change_set(_INPUT_PATH),执行 input_path.read_text()。如果文件不存在,会抛出 FileNotFoundError → 非零退出 → 沙箱返回 status="failed"。因此,容器评审会静默失败(被记录为沙箱失败,而非崩溃)。评估的容器路径会运行 run_agent review --sandbox container ...,这会导致失败 → _run_skill_checks 引发 RuntimeError("trusted_skill_execution_failed")。所以容器评估路径会失败。

但是 —— 这是故意的吗?默认的沙箱是 container,但测试和评估默认使用 locallocal 路径使用了 use_workspace_root=True,所以 cwd="."(工作空间根目录)且 args=[staged_skill.entrypoint] —— 等等,对于 localuse_workspace_root=Trueargs = [staged_skill.entrypoint],即 {workspace_skill_dir}/scripts/run_checks.py,且 cwd="."。在 local 工作空间中,工作空间根目录即 cwd。所以 work/inputs/diff.json 是相对于工作空间根目录的 —— 正确!因为 cwd="." 即工作空间根目录。

因此在 local 模式下,它之所以有效,是因为 cwd 是工作空间根目录。在 container 模式下,cwdworkspace_skill_dir 且路径错误。这是一个实际存在的 Bug,但仅影响容器运行时路径。鉴于容器是默认运行时 (run_agent review 默认使用 container),用户如果不使用 --sandbox local 运行 run_agent review --diff-file X,将会遇到沙箱失败。

然而,我应该谨慎:我不能百分之百确定 SDK 的 WorkspaceRunProgramSpec.cwd 语义 —— cwd 可能被解释为相对于工作空间根目录,而 run_program 可能实际上不会更改目录。如果 cwd 是相对于工作空间根目录的,scripts/run_checks.py 将解析为 skills/code-review/scripts/run_checks.py,而 work/inputs/diff.json 解析为 skills/code-review/work/inputs/diff.json —— 仍然是错误的。

这是基于推理而非经过验证的。提示要求如果无法从 diff 中验证,则使用“可能/疑似”。这是一个合理的警告。让我将其标记为警告,并附带关于无法验证 SDK 运行时 cwd 语义的说明。

实际上,更仔细地查看测试:test_registered_scripts_emit_sanitized_summary_and_findingscwd=workdir(根目录)运行 run_checks.py。这证实了脚本设计为从工作空间根目录运行。沙箱 build_run_spec 在容器模式下将 cwd 设置为 workspace_skill_dir —— 这与设计意图相矛盾。因此,容器路径下的 run_checks.py 将无法定位 work/inputs/diff.json。这是一个合理的正确性警告。

让我通过检查 stage_code_review_skill 的返回值来确认 workspace_skill_dir —— result.workspace_skill_dir 来自 SDK 的 CopySkillStager。这很可能是相对于工作空间根目录的。入口点使用 scripts/run_checks.py 中的 f"{result.workspace_skill_dir}/scripts/{definition.entrypoint}"。所以 workspace_skill_dir 是一个类似 skills/code-review 的目录。设置为 cwdrun_checks.py 读取相对路径 work/inputs/diff.json → 错误的位置。

我将报告此事作为警告(容器路径的输入路径不匹配),注明 SDK cwd 语义未经核实。让我完成最终确认。

让我再重新考虑一下:还有其他 cwd 为根目录的路径吗?只有本地 use_workspace_root=True。因此容器路径是损坏的。这是最有价值的发现。

让我最终完成报告。## 发现的问题

🚨 Critical

未发现必须修复的阻塞问题。

⚠️ Warning

  • examples/skills_code_review_agent/code_review/sandbox.py:6487(约 build_run_spec,对应 sandbox.py 新增文件 build_run_spec 函数):容器运行时下 run_checks.py 的工作目录与输入文件相对路径不一致

    • build_run_spec 在非 use_workspace_root 分支(即 container/cube 路径)把 cwd 设为 staged_skill.workspace_skill_dir(如 skills/code-review),但 _execute_async 通过 put_filesdiff.json 写到工作区根的 work/inputs/diff.json,而 run_checks.py 用相对路径 Path("work")/"inputs"/"diff.json" 读取。cwd 非 root 时该路径会解析到 skills/code-review/work/inputs/diff.json,文件不存在导致脚本非零退出、沙箱收敛为 status="failed"。集成测试 test_registered_scripts_emit_sanitized_summary_and_findings 实际是以 cwd=workdir(工作区根)运行脚本,恰好反证脚本设计上要求从 root 运行。建议容器路径也使用工作区根作为 cwd,或让 run_checks.py 从受控环境变量读取绝对输入路径。注:受限于 diff 无法验证 SDK WorkspaceRunProgramSpec.cwd 的确切解析语义,若 SDK 实际以工作区根为基准则该问题不成立。
  • examples/skills_code_review_agent/evaluate.py:8030_run_skill_checks,及 _evaluate_corpus/_observe_blind_spots 调用处):单条语料子进程超时会整体终止评测而非计入单条失败

    • _run_skill_checkssubprocess.run(..., timeout=...) 抛出 TimeoutExpiredSubprocessError 子类),向上冒泡到 mainexcept (..., subprocess.SubprocessError) 直接返回退出码 2,使 48+ 条语料中任意一条慢于 30s/110s 即判定整轮评测 failed,而非把该条记为失败继续统计指标。建议在 _run_skill_checks 或其调用处捕获 TimeoutExpired,把该 case 计为未命中后继续,提升 CI 稳定性。

总结

整体是一个安全设计严谨、防御充分的示例 Agent,凭证隔离、路径穿越/符号链接校验、参数化 SQL、脱敏与 fail-closed 治理均到位,未发现安全或核心逻辑层面的 Critical 问题。主要风险集中在容器运行时 run_checks.py 输入路径相对 cwd 不匹配(可能导致默认 container 入口静默失败)和评测单条超时放大为整轮失败两项稳定性问题,建议修复后再合入。

测试建议

  • 补充一条在真实/接近真实的 SDK workspace(非 fake runner)下以 --sandbox container 运行 run_checks 的端到端用例,验证 work/inputs/diff.json 能被脚本按 cwd 正确读到,覆盖上述路径不匹配风险。
  • evaluate.py 增加单条语料超时被隔离计 fail、而非终止整轮评测的场景测试。

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