Feat/code review app - #1
Conversation
![agentkit-veadk-studio[bot]](https://avatars.githubusercontent.com/u/277991807?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major]
veadk/cli/frontend_sandbox.py—_schedule_github_pull_request_review_message通过asyncio.create_task()创建后台任务，但未持有 task 引用。如果事件循环在服务器关闭时终止，可能出现 "Task was destroyed but it is pending" 警告。建议存储 task 引用，并在服务器关闭时优雅地取消它。 -
[Major]
veadk/cli/frontend_sandbox.py—_schedule_github_pull_request_review_message中的_run_review_message只捕获了SandboxError，但stream_message可能抛出其他异常（如asyncio.CancelledError之外的未预期异常），这些异常会被 asyncio 静默吞掉。建议加一个更宽泛的 except 兜底并记录日志。 -
[Minor]
veadk/cli/frontend_sandbox.py— PR 评审提示词模板_GITHUB_PULL_REQUEST_REVIEW_PROMPT硬编码在代码中（含中文要求）。建议提取到配置文件或常量模块，便于后续维护和国际化。 -
[Minor]
frontend/src/automations/pullRequestReview.ts—submit()函数现在直接抛出错误 "PR 自动评审已切换为 GitHub App 授权模式。" 这意味着旧的 workflow 生成流程被完全移除，已配置该自动化 workflow 的用户需要迁移。建议在 UI 中向用户展示迁移指引。 -
[Minor]
veadk/cli/github_app_pr_review.py—_app_jwt()中的iat硬编码为time.time() - 60，这虽然是常见的时钟偏差补偿，但 GitHub 官方推荐iat不超过当前时间 60 秒。建议将补偿值提取为常量（如JWT_IAT_OFFSET），并添加注释说明用途。 -
[Minor]
veadk/cli/frontend_sandbox.py— 移除了_agent_surface_capability和_valid_agent_surface_capability函数，但SandboxAgentSessionService的resolve_surface_proxy_target方法也被简化为resolve_proxy_target。如果外部有依赖这些能力签名的组件，存在兼容性风险。建议在 PR 描述或提交信息中记录此变更的动机。 -
[Nit]
veadk/tools/skills_tools/session_path.py—_get_base_path()中 Linux/macOS 的默认路径硬编码为/home/gem/veadk_skills/sessions。建议确认该路径是否在目标部署环境中可写，或者考虑使用$HOME环境变量。
Open Questions
veadk/cli/frontend_sandbox.py中新增的TosGitHubAppReviewRepositoryStore依赖 TOS 对象存储，但_github_app_review_store()在没有配置 bucket 时返回None，此时 webhook 端点会返回ignored状态。这是否需要在部署文档中明确说明？- 删除了
VeStackManagedTool相关的 managed tool 系统（包括create_managed_tool、list_managed_tools、delete_managed_tool等），这些是否已完全不再使用？如果还有外部依赖，需要确保兼容。
Verification
- Reviewed PR: #1
- Reviewed head SHA:
d800784f96c604384dbfd96d0af57345baeb18fa - 审查了
veadk/cli/github_app_pr_review.py（新建，507 行）、veadk/cli/frontend_sandbox.py（+418/-585）、veadk/tools/builtin_tools/remote_skills.py（+58/-3）、veadk/tools/skills_tools/session_path.py（+13/-8）、frontend/src/adk/githubIntegration.ts（+178/-1）、frontend/src/automations/pullRequestReview.ts（+10/-235）、frontend/src/automations/githubFields.ts（+0/-1）、frontend/src/automations/types.ts（+0/-2）、veadk/tools/skills_tools/bash_tool.py（+2/-2）、veadk/tools/skills_tools/file_tool.py（+3/-3）、pyproject.toml（+1）、tests/tools/test_skills_session_path.py（新建）、tests/tools/builtin_tools/test_remote_skills.py（+96/-6）等关键源文件。 - 未审查的变更：前端构建产物（
veadk/webui/assets/下的大量重命名 JS/CSS 文件）、设计图（data/design-lark-chart/下的 .jpg 文件）、reference/actb-mono符号链接、uv.lock依赖锁定文件，以及tests/cli/test_frontend_sandbox.py等大型测试文件。这些文件要么是二进制/构建产物，要么因篇幅过长未逐行检查。 - CI 状态：之前的 CI 运行中 License-Check 和 test (3.10) 曾失败，但最新的 CI 正在运行中（pre-commit、test、License-Check、gitleaks、gate 等均在 IN_PROGRESS）。
- 已使用
gh pr view和gh api获取 PR 元数据和文件列表。 - 未直接克隆仓库，使用 GitHub API 获取文件内容进行审查。
There was a problem hiding this comment.
Findings
Critical
-
权限绕过：手动PR评审端点无仓库访问控制
- 位置：
veadk/cli/frontend_sandbox.py:3809-3845 - 问题：
/web/github/pull-request-reviews端点允许任何认证用户为任意PR创建评审会话，但没有像webhook端点那样检查该仓库是否在管理员启用的评审列表中，也没有验证当前用户是否有权限访问该仓库。攻击者可以利用此端点为GitHub App安装下的任意仓库生成installation token。 - 影响：横向越权，非授权用户可以访问任意仓库的token并通过沙箱执行操作。
- 修复：在手动创建评审会话前，必须验证：(1) 目标仓库在管理员启用的评审列表中；(2) 当前用户对该仓库有访问权限。
- 位置：
-
GitHub token权限过大且直接暴露给沙箱
- 位置：
veadk/cli/frontend_sandbox.py:3605 - 问题：GitHub App installation token被直接通过
GITHUB_TOKEN环境变量注入到沙箱中，agent可以直接使用该token执行任意GitHub API操作。prompt中的"不要修改仓库"只是对LLM的软约束，没有技术层面的强制限制。 - 影响：token默认拥有GitHub App配置的全部权限（可能包括代码写入、PR合并、Issue修改等），LLM幻觉或prompt注入可能导致仓库被意外修改或恶意操作。
- 修复：(1) 在生成installation token时，通过
permissions参数将token限制为只读权限 + PR评论写入权限；(2) 不要直接暴露原始token，而是通过一个代理层封装仅评审所需的API操作。
- 位置：
Major
- 硬编码开发者个人主目录路径
- 位置：
veadk/tools/skills_tools/session_path.py:25 - 问题：
DEFAULT_SKILLS_WORK_DIR被硬编码为/home/gem/veadk_skills/sessions，这是开发者个人的主目录路径，在其他环境（生产环境、其他开发者机器）中会导致路径不存在或权限错误。 - 影响：会话目录创建失败，功能不可用。
- 修复：使用跨平台的合理默认路径，如
~/.cache/veadk/sessions或使用tempfile.gettempdir()下的目录，与原逻辑保持一致。
- 位置：
Minor
-
新增测试文件
tests/tools/test_skills_session_path.py缺少Apache License头，这是CI中License-Check失败的直接原因。 -
空目录
reference/actb-mono被意外提交到仓库，没有任何内容，建议添加.gitkeep或移除。 -
Webhook触发的自动评审没有速率限制，恶意用户可以通过频繁提交PR创建大量沙箱会话导致资源耗尽。
CI Status
- License-Check: FAILURE - 主要是新增文件缺少license头
- Unit Tests (3.10/3.12): FAILURE - 建议在合并前修复测试失败
- gitleaks/pre-commit: SUCCESS
Verification
- 评审commit: d800784
- 检查了所有Python和TypeScript源文件变更
- 验证了webhook签名验证逻辑正确
- 未运行本地测试，仅静态代码审查
| if match is None: | ||
| raise SandboxValidationError("请输入完整的 GitHub Pull Request URL。") | ||
| owner, repo, _number = match.groups() | ||
| installation_token = await _github_app_installation_token_for_pull_request( |
There was a problem hiding this comment.
[Critical] 此处直接为用户提供的PR URL生成installation token，但没有检查该仓库是否在管理员启用的评审仓库列表中，也没有验证用户是否有权限访问该仓库。任何登录用户都可以使用此接口为GitHub App安装下的任意仓库获取token，造成横向越权。请参考webhook处理逻辑（3768-3780行）加入相同的仓库启用检查，并增加用户权限校验。
| creator_name, | ||
| False, | ||
| envs={ | ||
| "GITHUB_TOKEN": installation_token, |
There was a problem hiding this comment.
[Critical] GitHub App installation token被直接注入到沙箱环境变量中，沙箱内的所有进程（包括agent执行的任意命令）都可以读取并使用这个token。Prompt中的"不要修改仓库"只是对LLM的软约束，无法防止LLM被prompt注入或产生幻觉时执行写操作。建议：1) 生成token时通过permissions参数限制为只读+PR评论权限；2) 通过服务端代理封装评审所需的API，不直接暴露原始token给沙箱。
|
|
||
| # Cache of initialized session paths to avoid re-creating symlinks | ||
| _session_path_cache: dict[str, Path] = {} | ||
| DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions") |
There was a problem hiding this comment.
[Major] 此处硬编码了/home/gem/veadk_skills/sessions作为默认路径，这是开发者个人主目录，在生产环境或其他机器上会导致路径不存在或权限错误。建议使用更通用的默认路径，如Path.home()/'.cache'/'veadk'/'sessions'，或者恢复原来的/tmp/veadk默认路径。
| @@ -0,0 +1,28 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
[Minor] 此文件缺少Apache License头，这是CI中License-Check检查失败的原因之一。请参考其他Python文件添加标准的Apache 2.0许可证头。
There was a problem hiding this comment.
Code Review Findings (HEAD: f65a3cd)
This review covers the latest commit f65a3cd, including fixes from the previous review round.
Critical
1. 权限绕过：手动PR评审端点仍缺少仓库访问控制检查
- 位置:
veadk/cli/frontend_sandbox.py:4013-4058(_start_github_pull_request_review) - 问题: 对比 webhook 端点（第3930-3946行）会验证
event.repository是否在管理员启用的评审仓库白名单中，但手动评审端点完全没有此检查。任何已认证用户（不只是管理员）都可以通过传入任意公开/私有仓库的PR URL，为GitHub App安装下的任意仓库获取installation token并创建沙箱会话。 - 影响: 横向越权 - 攻击者可以访问未授权的私有仓库代码、在沙箱中执行任意操作，甚至通过token修改仓库内容。
- 修复: 在第4029行获取token之前，必须：
- 检查当前用户是否为管理员（或添加明确的RBAC权限控制）
- 验证目标仓库
{owner}/{repo}是否通过store.enabled_repositories()在白名单中 - 如果store未配置或仓库未启用，返回403错误
2. GitHub Installation Token 权限仍未受限，直接暴露给沙箱
- 位置:
veadk/cli/github_app_pr_review.py:340-349(installation_token方法) +frontend_sandbox.py:3663 - 问题: 调用
/app/installations/{installation_id}/access_tokens时未传递permissions参数，token默认拥有GitHub App配置的全部权限。token被直接通过GITHUB_TOKEN环境变量注入沙箱，prompt中的"不要修改仓库"只是LLM软约束，没有技术强制力。 - 影响: LLM幻觉、prompt注入或恶意利用都可能导致仓库代码被修改、PR被合并、issue被删除等破坏性操作。
- 修复:
- 在
installation_token()请求body中添加最小权限原则的permissions：pull_requests: read+pull_requests: write（仅用于提交评审评论）contents: read（读取代码）metadata: read
- 考虑不要直接暴露原始token，而是封装一个代理层只暴露评审所需的API
- 在
Major
3. 硬编码开发者个人主目录路径仍未修复
- 位置:
veadk/tools/skills_tools/session_path.py:23 - 问题:
DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions")仍然存在。/home/gem是开发者个人目录，在生产环境或其他开发者机器上不存在，会导致目录创建失败或权限错误。 - 影响: 会话路径初始化失败，skills工具无法正常工作。
- 修复: Linux/macOS默认路径应使用
Path.home() / ".cache" / "veadk" / "sessions"或Path(tempfile.gettempdir()) / "veadk-sessions"，Windows保持现有逻辑即可。
4. 新增测试文件仍然缺少Apache License头
- 位置:
tests/tools/test_skills_session_path.py - 问题: 文件开头直接是
from pathlib import Path，没有标准的Apache 2.0 License头。CI的License-Check检查会因此失败。 - 修复: 添加与其他测试文件一致的License头。
Minor
5. 后台任务没有引用持有且异常捕获不完整
- 位置:
veadk/cli/frontend_sandbox.py:3738 - 问题:
asyncio.create_task(_run_review_message())创建的后台任务没有保存引用，服务器关闭时可能出现"Task was destroyed but it is pending"警告。另外_run_review_message只捕获了SandboxError，其他异常（如网络错误、JSON解析错误）会被asyncio静默吞掉。 - 修复: 存储task引用到一个set中，在app shutdown时优雅取消；添加宽泛的
except Exception兜底并记录error日志。
6. Webhook自动评审没有速率限制
- 问题: 恶意用户可以通过频繁创建/更新PR触发大量评审任务，耗尽沙箱资源。
- 修复: 添加基于installation_id+repository的速率限制（如每仓库每5分钟最多1个并发评审）。
7. JWT iat偏移量建议提取为常量
- 位置:
veadk/cli/github_app_pr_review.py:520 - 问题:
issued_at = int(time.time()) - 60硬编码60秒时钟偏差补偿，建议提取为常量JWT_CLOCK_SKEW_SECONDS = 60并添加注释。
已修复验证
- ✅ Pagination UI: 最新提交修复了GitHub设置页面分页按钮在无相邻页面时的隐藏问题
- ✅ PR Review表单label: 已移除不必要的输入标签
- ✅ 评审设置加载错误: 仓库列表未加载前避免设置错误提示
Open Questions
- 删除的
VeStackManagedTool系统是否确认无外部依赖？ reference/actb-mono空目录是否应该移除或添加.gitkeep？
Verification
- Reviewed PR: #1
- Reviewed head SHA:
f65a3cd0572068c5406bd982b8c31493f8afff72 - Diff since previous review (d800784 → f65a3cd): +1129/-331 lines across source/test files
- Files examined:
veadk/cli/github_app_pr_review.py,veadk/cli/frontend_sandbox.py,veadk/cli/cli_frontend.py,veadk/tools/skills_tools/session_path.py,tests/tools/test_skills_session_path.py - CI Status: All checks IN_PROGRESS at review time
- Note: Frontend build assets and mermaid diagram chunks not reviewed (binary/generated files)
There was a problem hiding this comment.
Code Review Findings (HEAD: 6f1a669)
本评审针对最新提交 6f1a669，覆盖前三轮评审后剩余的问题。前三轮指出的所有 Critical 和 Major 问题均未修复，最新提交仅修改了前端 UI（完成后隐藏会话操作按钮）。
Critical
1. 权限绕过：手动PR评审端点仍缺少仓库访问控制检查（未修复）
- 位置:
veadk/cli/frontend_sandbox.py:4013-4070(_start_github_pull_request_review) - 问题: 对比 webhook 端点（第3924-3946行）会验证
event.repository是否在管理员启用的评审仓库白名单中，但手动评审端点完全没有此检查。任何已认证用户都可以通过传入任意 PR URL，为 GitHub App 安装下的任意仓库获取 installation token 并创建沙箱会话。 - 影响: 横向越权 - 攻击者可以访问未授权的私有仓库代码、在沙箱中执行任意操作，甚至通过 token 修改仓库内容。
- 修复: 在获取 token 之前必须添加与 webhook 一致的检查：
- 验证目标仓库
{owner}/{repo}是否通过store.enabled_repositories()在白名单中 - 如果 store 未配置或仓库未启用，返回 403 错误
- 验证目标仓库
2. GitHub Installation Token 权限仍未受限，直接暴露给沙箱（未修复）
- 位置:
veadk/cli/github_app_pr_review.py:615-627(installation_token方法) - 问题: 调用
/app/installations/{installation_id}/access_tokens时未传递permissions参数，token 默认拥有 GitHub App 配置的全部权限。token 被直接通过GITHUB_TOKEN环境变量注入沙箱，prompt 中的"不要修改仓库"只是 LLM 软约束，没有技术强制力。 - 影响: LLM 幻觉、prompt 注入或恶意利用都可能导致仓库代码被修改、PR 被合并、issue 被删除等破坏性操作。
- 修复:
- 在
installation_token()请求 body 中添加最小权限原则的permissions：pull_requests: read+pull_requests: write（仅用于提交评审评论）contents: read（读取代码）metadata: read
- 建议不要直接暴露原始 token，而是封装一个代理层只暴露评审所需的 API
- 在
Major
3. 硬编码开发者个人主目录路径（未修复）
- 位置:
veadk/tools/skills_tools/session_path.py:23 - 问题:
DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions")仍然存在。/home/gem是开发者个人目录，在生产环境或其他开发者机器上不存在，会导致目录创建失败或权限错误。 - 影响: 会话路径初始化失败，skills 工具无法正常工作。
- 修复: Linux/macOS 默认路径应使用
Path.home() / ".cache" / "veadk" / "sessions"或Path(tempfile.gettempdir()) / "veadk-sessions"。
4. 新增测试文件仍然缺少 Apache License 头（未修复，CI 失败直接原因）
- 位置:
tests/tools/test_skills_session_path.py - 问题: 文件开头直接是
from pathlib import Path，没有标准的 Apache 2.0 License 头。这是 CI 中 License-Check FAILURE 的直接原因。 - 修复: 添加与其他测试文件一致的 License 头（参考其他测试文件开头的版权声明）。
Minor
5. 后台任务没有引用持有且异常捕获不完整（未修复）
- 位置:
veadk/cli/frontend_sandbox.py:3742 - 问题:
asyncio.create_task(_run_review_message())创建的后台任务没有保存引用，服务器关闭时可能出现"Task was destroyed but it is pending"警告。另外_run_review_message只捕获了SandboxError，其他异常（如网络错误、JSON 解析错误）会被 asyncio 静默吞掉。 - 修复: 存储 task 引用到一个集合中，在 app shutdown 时优雅取消；添加宽泛的
except Exception兜底并记录 error 日志。
6. Webhook 自动评审没有速率限制（未修复）
- 问题: 恶意用户可以通过频繁创建/更新 PR 触发大量评审任务，耗尽沙箱资源。
- 修复: 添加基于 installation_id + repository 的速率限制（如每仓库每 5 分钟最多 1 个并发评审）。
7. JWT iat 偏移量建议提取为常量（未修复）
- 位置:
veadk/cli/github_app_pr_review.py:739 - 问题:
issued_at = int(time.time()) - 60硬编码 60 秒时钟偏差补偿，建议提取为常量JWT_CLOCK_SKEW_SECONDS = 60并添加注释说明用途。
CI Status
- License-Check: FAILURE - 主要原因是新增测试文件缺少 license 头
- build: FAILURE - 需要检查构建日志
- pre-commit/gitleaks/codex-smoke/gate: SUCCESS
- test (3.10/3.12): PENDING
Verification
- Reviewed PR: #1
- Reviewed head SHA:
6f1a6690ab3765769c20ceb93ef772eed08a643c - Diff since previous review (f65a3cd → 6f1a669): 仅前端 UI 变更（评审完成后隐藏会话操作按钮），核心后端安全问题均未修复
- Files examined:
veadk/cli/github_app_pr_review.py,veadk/cli/frontend_sandbox.py,veadk/tools/skills_tools/session_path.py,tests/tools/test_skills_session_path.py - Note: 前端构建产物、mermaid diagram chunks、设计图片未审查（二进制/生成文件）
There was a problem hiding this comment.
Code Review Findings (HEAD: dcf3ee7)
本评审针对最新提交 dcf3ee7（"delete useless file"，仅删除了 4 个设计图 JPG 文件）。前四轮评审指出的所有 Critical 和 Major 问题均未修复，且本轮发现一个新的 CI 构建阻塞问题（gitlink 损坏）。
Critical
1. [Critical] 权限绕过：手动PR评审端点仍缺少仓库访问控制检查（未修复，第五轮）
- 位置：
veadk/cli/frontend_sandbox.py:4029（见内联评论） - Webhook 端点（第3924行）正确验证了
enabled_repositories()白名单，但手动评审端点完全跳过此检查，且无管理员权限校验。任何已认证用户可通过传入任意 PR URL 获取 GitHub App installation token 并创建沙箱会话。
2. [Critical] GitHub Installation Token 权限仍未受限，原始 token 直接暴露给沙箱（未修复，第五轮）
- 位置：
veadk/cli/github_app_pr_review.py:617和veadk/cli/frontend_sandbox.py:3663（见内联评论） - 获取 token 时未传递
permissions参数，token 继承 App 全部权限；原始 token 通过GITHUB_TOKEN环境变量直接注入沙箱，prompt 中的"不要修改仓库"只是 LLM 软约束。
Major
3. [Major] 新增 gitlink 导致 CI build 失败（新发现）
reference/actb-mono以 gitlink（mode 160000，指向 commit7d6536a8）方式被提交，但仓库中没有.gitmodules文件。CI checkout 阶段执行git submodule foreach时报错fatal: No url found for submodule path 'reference/actb-mono' in .gitmodules，导致 build job 直接失败。- 修复：删除该 gitlink 条目（
git rm --cached reference/actb-mono），若需保留空目录请添加.gitkeep；若确实需要子模块，请补全.gitmodules配置。
4. [Major] 硬编码开发者个人主目录路径（未修复，第五轮）
- 位置：
veadk/tools/skills_tools/session_path.py:23（见内联评论） DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions")在非开发者环境中不存在，将导致目录创建失败。
5. [Major] 新增测试文件缺少 Apache License 头导致 License-Check 失败（未修复，第五轮）
- 位置：
tests/tools/test_skills_session_path.py:1（见内联评论） - CI 明确报告该文件缺少 license header（1006 文件中唯一无效的文件）。
Minor
6. 后台任务 asyncio.create_task(_run_review_message())（第3742行）未持有引用，服务器关闭时可能出现 "Task was destroyed but it is pending" 警告；且 _run_review_message 仅捕获 SandboxError，其他异常会被 asyncio 静默吞掉。建议保存 task 引用到集合中，shutdown 时优雅取消，并添加 except Exception 兜底记录 error 日志。（见内联评论）
7. Webhook 自动评审无速率限制，恶意用户可通过频繁创建/更新 PR 耗尽沙箱资源。建议添加基于 installation_id + repository 的并发/频率限制。
8. JWT iat 偏移量硬编码为 time.time() - 60（github_app_pr_review.py:739），建议提取为常量 JWT_CLOCK_SKEW_SECONDS = 60 并添加注释。
CI Status
- License-Check: FAILURE —
tests/tools/test_skills_session_path.py缺少 license header - build: FAILURE —
reference/actb-monogitlink 无.gitmodules配置导致 submodule 错误 - pre-commit: PASS | harness-sidecar-release-gate/gate: PASS
- test (3.10/3.12): 未运行（build 失败前置阻塞）
- Merge status: CONFLICTING（与 main 分支存在合并冲突）
Verification
- Reviewed PR: #1
- Reviewed head SHA:
dcf3ee734b561502b5ea6d14192b891011aaaa9a - Diff since previous review (6f1a669 → dcf3ee7): 仅删除 4 个设计图 JPG 文件（
data/design-lark-chart/review-poc-sync/*.jpg），核心后端安全/正确性问题均未修复 - 通过
gh repo clone+git fetch pull/1/head获取完整代码进行逐文件审查 - 已检查 CI 失败日志（build 和 License-Check）
- 前端构建产物（
veadk/webui/assets/）、uv.lock 未逐行审查（生成的/锁定文件）
| if match is None: | ||
| raise SandboxValidationError("请输入完整的 GitHub Pull Request URL。") | ||
| owner, repo, _number = match.groups() | ||
| installation_token = await _github_app_installation_token_for_pull_request( |
There was a problem hiding this comment.
[Critical] 此处直接调用 _github_app_installation_token_for_pull_request(owner, repo) 获取 installation token，没有像 webhook 端点（第3924-3946行）那样验证 {owner}/{repo} 是否在 store.enabled_repositories() 白名单中，也没有校验当前用户是否为管理员。
影响：横向越权。任何已认证用户可传入任意仓库（包括私有仓库）的 PR URL，获取该仓库的 installation token 并在沙箱中执行操作，包括读取/修改代码。
修复：在获取 token 之前，必须添加与 webhook 一致的检查：
- 检查
store是否可用； - 验证
f"{owner}/{repo}".casefold()是否在启用仓库集合中； - 建议同时校验
_is_admin(request)或添加明确的 RBAC 权限控制； - 不在白名单中返回 403。
| self._timeout = timeout | ||
|
|
||
| async def installation_token(self, installation_id: int) -> str: | ||
| payload = await self._request( |
There was a problem hiding this comment.
[Critical] 调用 /app/installations/{installation_id}/access_tokens 时未在请求 body 中传递 permissions 参数，生成的 token 将继承 GitHub App 配置的全部权限（可能包括 contents:write、pull_requests:write、issues:write 等）。
结合 frontend_sandbox.py:3663 将此 token 直接作为 GITHUB_TOKEN 环境变量注入沙箱，且仅靠 prompt 中的约束来限制 LLM 行为，无法在技术层面阻止 token 被用于破坏性操作（合并 PR、推送代码、删除 issue 等）。
修复建议：
- 在 POST body 中添加最小权限
permissions：仅需contents: read、metadata: read、pull_requests: read+pull_requests: write（评论需要写权限）； - 考虑不直接暴露原始 token，而是在后端封装一个仅暴露评审所需 API（读 PR、提交评论）的代理层。
| @@ -21,6 +22,16 @@ | |||
|
|
|||
| # Cache of initialized session paths to avoid re-creating symlinks | |||
There was a problem hiding this comment.
[Major] DEFAULT_SKILLS_WORK_DIR 硬编码为 /home/gem/veadk_skills/sessions，这是开发者个人主目录路径。在生产部署、其他开发者机器或 CI 环境中，该路径要么不存在，要么当前用户无写入权限，将导致会话目录创建失败、skills 工具不可用。
对应的测试用例（tests/tools/test_skills_session_path.py:9）也断言了这个硬编码路径，进一步固化了问题。
修复：Linux/macOS 默认路径应使用 Path.home() / ".cache" / "veadk" / "sessions" 或 Path(tempfile.gettempdir()) / "veadk-sessions"，与 Windows 分支逻辑保持一致。
| @@ -0,0 +1,28 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
[Major] 文件缺少 Apache License 2.0 版权头。CI License-Check（skywalking-eyes/header）明确报告该文件为 1006 个被检查文件中唯一缺少有效 license header 的文件，直接导致 License-Check job FAILURE。
修复：在文件开头添加与其他测试文件一致的 Apache 2.0 License 头（参考仓库中其他 tests/ 下文件的开头格式）。
| _safe_error_message(error), | ||
| ) | ||
|
|
||
| asyncio.create_task(_run_review_message()) |
There was a problem hiding this comment.
[Minor] asyncio.create_task(_run_review_message()) 创建的后台任务没有保存引用。根据 Python asyncio 文档，未持有引用的 Task 可能在任何时候被垃圾回收，导致 "Task was destroyed but it is pending" 警告并中断执行。
此外，_run_review_message（第3725行）只捕获了 SandboxError，其他异常（网络超时、JSON 解析错误、stream_message 内部错误等）会导致 Task 静默失败，评审状态永远停留在 "started"。
修复：
- 将 task 保存到模块级
set中防止被 GC，并在 app shutdown 时统一取消； - 添加
except Exception as error兜底分支，调用_update_status("failed", ...)并记录logger.exception(...)。
There was a problem hiding this comment.
Code Review Findings (HEAD: 9630bc0 — 第六轮评审)
本评审针对最新提交 9630bc0（两次合并 origin/main 后的快照）。自上一轮评审（dcf3ee7）以来，两次合并仅带入了 main 分支的改动（managed tool disk storage 能力、前端构建产物重命名等），前五轮指出的所有 Critical 和 Major 问题均未修复。
Critical（未修复，第六轮）
1. 权限绕过：手动 PR 评审端点仍缺少仓库访问控制检查
- 位置：
veadk/cli/frontend_sandbox.py:4053（见内联评论） - Webhook 端点（第3948行）正确验证了
enabled_repositories()白名单，但手动评审端点在第4053行直接调用_github_app_installation_token_for_pull_request(owner, repo)获取 token 并创建沙箱会话，完全跳过了仓库白名单检查。 - 影响：横向越权，任何已认证用户可传入任意 PR URL 获取 GitHub App installation token 并执行沙箱操作。
- 修复：在第4053行获取 token 前，添加与 webhook 端点一致的
enabled_repositories()白名单检查。
2. GitHub Installation Token 权限仍未受限，原始 token 直接暴露给沙箱
- 位置：
veadk/cli/github_app_pr_review.py:615（见内联评论） - 获取 token 时未传递
permissions参数，token 继承 App 全部权限；原始 token 通过GITHUB_TOKEN环境变量直接注入沙箱（frontend_sandbox.py:3687），prompt 中的"不要修改仓库"只是 LLM 软约束。 - 影响：LLM 幻觉或 prompt 注入可能导致仓库代码被修改、PR 被合并等破坏性操作。
Major（未修复，第六轮）
3. Gitlink 导致 CI build 失败
- 位置：
reference/actb-mono（mode 160000，指向 commit7d6536a8） - 仓库中没有
.gitmodules文件，CI checkout 执行git submodule foreach时报错fatal: No url found for submodule path 'reference/actb-mono' in .gitmodules，导致 build 失败。 - 修复：
git rm --cached reference/actb-mono删除该 gitlink；若需保留空目录请添加.gitkeep。
4. 硬编码开发者个人主目录路径
- 位置：
veadk/tools/skills_tools/session_path.py:25（见内联评论） DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions")在非开发者环境不存在。- 测试文件
test_skills_session_path.py甚至断言了这个硬编码路径（第10行），进一步固化了该问题。
5. 新增测试文件缺少 Apache License 头导致 License-Check 失败
- 位置：
tests/tools/test_skills_session_path.py:1（见内联评论） - 这是 CI License-Check FAILURE 的直接原因（1006 个文件中唯一无效的文件）。
Minor（未修复，第六轮）
6. 后台任务 asyncio.create_task(_run_review_message())（frontend_sandbox.py:3766，见内联评论）未持有引用，服务器关闭时可能出现 "Task was destroyed but it is pending" 警告；且 _run_review_message 仅捕获 SandboxError，其他异常会被 asyncio 静默吞掉。
7. Webhook 自动评审无速率限制，恶意用户可通过频繁创建/更新 PR 耗尽沙箱资源。
8. JWT iat 偏移量硬编码为 time.time() - 60（github_app_pr_review.py:739，见内联评论），建议提取为常量 JWT_CLOCK_SKEW_SECONDS = 60 并添加注释。
CI Status
- License-Check: FAILURE —
tests/tools/test_skills_session_path.py缺少 license header - gate: FAILURE — build 被 gitlink 问题阻塞
- pre-commit: PASS | gitleaks: PASS | codex-smoke: PASS
- test (3.10/3.12): PENDING（gate 失败前置阻塞）
Verification
- Reviewed PR: #1
- Reviewed head SHA:
9630bc0d64ff02d6b0395b5370bf3026966d902c - Diff since previous review (dcf3ee7 → 9630bc0): 两次 origin/main 合并带入的 main 分支变更（managed tool disk storage 能力 + 前端构建产物更新），PR 核心功能文件无新修复
- 通过
gh repo clone+gh pr checkout获取完整代码逐文件审查 - 前端构建产物（
veadk/webui/assets/）、uv.lock、package-lock.json未逐行审查（生成的/锁定文件）
| if match is None: | ||
| raise SandboxValidationError("请输入完整的 GitHub Pull Request URL。") | ||
| owner, repo, _number = match.groups() | ||
| installation_token = await _github_app_installation_token_for_pull_request( |
There was a problem hiding this comment.
[Critical] 此处直接调用 _github_app_installation_token_for_pull_request(owner, repo) 获取 GitHub App installation token，然后在第4058行创建沙箱会话，但没有像 webhook 端点（第3948行）那样检查目标仓库是否在管理员启用的白名单 enabled_repositories() 中。
任何已认证用户都可以传入任意仓库（包括未授权的私有仓库）的 PR URL，获取该仓库的 installation token 并在沙箱中执行操作，构成横向越权漏洞。
修复建议：在此行之前添加与 webhook 端点一致的检查逻辑：
store = _github_app_review_store()
if store is None:
raise SandboxError("评审设置不可用")
enabled = await store.enabled_repositories()
repo_full = f"{owner}/{repo}"
if repo_full.casefold() not in {r.casefold() for r in enabled}:
raise SandboxError(f"仓库 {repo_full} 未启用 PR 评审")| self._api_root = api_root.rstrip("/") | ||
| self._timeout = timeout | ||
|
|
||
| async def installation_token(self, installation_id: int) -> str: |
There was a problem hiding this comment.
[Critical] installation_token() 方法向 /app/installations/{installation_id}/access_tokens 发送 POST 请求时，没有在请求 body 中传递 permissions 参数，因此返回的 token 默认拥有 GitHub App 配置的全部权限（可能包括 contents:write、pull_requests:write、issues:write 等）。
随后此 token 被通过 GITHUB_TOKEN 环境变量直接注入沙箱（frontend_sandbox.py:3687），LLM prompt 中的"不要修改仓库"只是软约束，没有技术强制力。
修复建议：
- 在请求 body 中添加最小权限
permissions：body = { "permissions": { "contents": "read", "pull_requests": "write", "metadata": "read" } }
- 考虑不直接暴露原始 token，而是封装代理层只暴露评审所需的 API 操作。
|
|
||
| # Cache of initialized session paths to avoid re-creating symlinks | ||
| _session_path_cache: dict[str, Path] = {} | ||
| DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions") |
There was a problem hiding this comment.
[Major] DEFAULT_SKILLS_WORK_DIR 被硬编码为 /home/gem/veadk_skills/sessions，这是开发者个人的主目录路径。在生产环境或其他开发者机器上，/home/gem 目录不存在或无写权限，会导致会话目录创建失败、skills 工具不可用。
另外，测试文件 tests/tools/test_skills_session_path.py:10 还断言了这个硬编码路径，进一步固化了该问题。
修复建议：Linux/macOS 默认路径应改为：
DEFAULT_SKILLS_WORK_DIR = Path.home() / ".cache" / "veadk" / "sessions"
# 或使用 tempfile: Path(tempfile.gettempdir()) / "veadk-sessions"| @@ -0,0 +1,28 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
[Major] 此文件缺少 Apache 2.0 License 头，是 CI License-Check FAILURE 的直接原因（1006 个文件中唯一无效的文件）。
修复：参考项目中其他测试文件，在文件开头添加标准的 Apache 2.0 版权声明头（例如参考 tests/tools/builtin_tools/test_remote_skills.py 的开头）。
| _safe_error_message(error), | ||
| ) | ||
|
|
||
| asyncio.create_task(_run_review_message()) |
There was a problem hiding this comment.
[Minor] asyncio.create_task(_run_review_message()) 创建的后台任务没有保存引用。根据 Python asyncio 文档，未持有引用的 Task 可能在执行完成前被垃圾回收，导致 "Task was destroyed but it is pending!" 警告，且任务中的异常（除了已捕获的 SandboxError）会被 asyncio 的 exception handler 静默吞掉。
修复建议：
- 将 task 引用存储到一个模块级别的
set()中防止被 GC； - 添加
except Exception兜底捕获并记录 error 日志：
_background_tasks: set[asyncio.Task] = set()
task = asyncio.create_task(_run_review_message())
_background_tasks.add(task)
task.add_done_callback(_background_tasks.discard)| raise GitHubAppReviewError( | ||
| "缺少 PyJWT 依赖，无法生成 GitHub App JWT。" | ||
| ) from error | ||
| issued_at = int(time.time()) - 60 |
There was a problem hiding this comment.
[Minor] issued_at = int(time.time()) - 60 中的 60 秒时钟偏差补偿值是硬编码的魔法数字。建议提取为常量 JWT_CLOCK_SKEW_SECONDS = 60 并添加注释说明其用途（补偿客户端与 GitHub 服务器之间的时钟偏差）。
There was a problem hiding this comment.
Code Review Findings (HEAD: ff84c73 — 第七轮评审)
本评审针对最新提交 ff84c73（"fix(ci): resolve sandbox stream schema and lockfile issues"）。自上一轮评审（9630bc0）以来，最新提交修复了测试文件 License 头问题和 sandbox stream schema 兼容性问题，License-Check 和 gate CI 检查已通过。但前六轮指出的 2 个 Critical 安全问题和核心 Major 问题均未修复。
✅ 已修复
- 测试文件缺少 Apache License 头：
tests/tools/test_skills_session_path.py已添加标准 License 头，CI License-Check 现已通过 - Sandbox stream schema 兼容性：
stream_message接口添加了缺失的output_schema参数
Critical（未修复，第七轮）
1. 权限绕过：手动 PR 评审端点仍缺少仓库访问控制白名单检查
- 位置：
veadk/cli/frontend_sandbox.py:4056（见内联评论） - Webhook 端点（第 3951 行）正确验证了
enabled_repositories()白名单，但手动评审端点在第 4056 行直接调用_github_app_installation_token_for_pull_request(owner, repo)获取 token 并创建沙箱会话，完全跳过了仓库白名单检查和管理员权限校验。 - 影响：横向越权 - 任何已认证用户可传入任意 PR URL（包括私有仓库），获取 GitHub App installation token 并在沙箱中执行任意操作。
- 修复：在第 4056 行获取 token 前，必须添加与 webhook 端点一致的
enabled_repositories()白名单检查。
2. GitHub Installation Token 权限仍未受限，原始 token 直接暴露给沙箱
- 位置：
veadk/cli/github_app_pr_review.py:617（见内联评论）+veadk/cli/frontend_sandbox.py:3690 - 获取 token 时调用
POST /app/installations/{installation_id}/access_tokens未传递permissions参数，token 继承 GitHub App 配置的全部权限；原始 token 通过GITHUB_TOKEN环境变量直接注入沙箱，prompt 中的"不要修改仓库"只是 LLM 软约束，没有技术强制力。 - 影响：LLM 幻觉、prompt 注入或恶意利用都可能导致仓库代码被修改、PR 被合并、issue 被删除等破坏性操作。
- 修复：(1) 在 access_tokens 请求 body 中添加最小权限
permissions: {"pull_requests": "write", "contents": "read", "metadata": "read"}；(2) 考虑封装代理层而不是直接暴露原始 token。
Major（未修复，第七轮）
3. 硬编码开发者个人主目录路径
- 位置：
veadk/tools/skills_tools/session_path.py:25（见内联评论） DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions")是开发者个人目录，在生产环境或其他开发者机器上不存在，会导致目录创建失败或权限错误。代码注释中也硬编码了该路径。- 影响：Skills 会话路径初始化失败，相关工具不可用。
- 修复：Linux/macOS 默认路径应使用
Path.home() / ".cache" / "veadk" / "sessions"或Path(tempfile.gettempdir()) / "veadk-sessions"。
Minor（未修复，第七轮）
4. 后台任务 asyncio.create_task(_run_review_message())（frontend_sandbox.py:3769，见内联评论）未持有引用，服务器关闭时可能出现 "Task was destroyed but it is pending" 警告；且 _run_review_message 仅捕获 SandboxError，其他异常会被 asyncio 静默吞掉。参考第 1045-1047 行已有正确的 _background_tasks 管理模式，应保持一致。
5. Webhook 自动评审无速率限制，恶意用户可通过频繁创建/更新 PR 触发大量评审任务，耗尽沙箱资源。
6. JWT iat 偏移量硬编码为 int(time.time()) - 60（github_app_pr_review.py:739，见内联评论），建议提取为常量 JWT_CLOCK_SKEW_SECONDS = 60 并添加注释说明用途。
CI Status
- License-Check: PASS ✅（已修复）
- pre-commit/gitleaks/codex-smoke/gate: PASS ✅
- build: FAILURE
- test (3.10/3.12): PENDING（前置依赖）
- reference/actb-mono gitlink：仍以 mode 160000 形式存在且无
.gitmodules，但 gate 检查现已通过，不再阻塞合并（仍建议清理）
Verification
- Reviewed PR: #1
- Reviewed head SHA:
ff84c73d626c0f40cc217cb5e1f5e9da5763fb9e - Diff since previous review (9630bc0 → ff84c73): 4 个文件变更，修复了 License 头和 sandbox stream schema 问题，但核心后端安全问题均未修复
- 通过
gh repo clone+git fetch pull/1/head获取完整代码逐文件审查 - 已检查 CI 最新状态
- 前端构建产物（
veadk/webui/assets/）、uv.lock、package-lock.json未逐行审查（生成的/锁定文件）
| if match is None: | ||
| raise SandboxValidationError("请输入完整的 GitHub Pull Request URL。") | ||
| owner, repo, _number = match.groups() | ||
| installation_token = await _github_app_installation_token_for_pull_request( |
There was a problem hiding this comment.
[Critical] 在获取 GitHub installation token 前缺少仓库白名单检查。对比 Webhook 端点（第 3951 行）会先验证 store.enabled_repositories() 中是否包含目标仓库，但手动端点此处直接调用 _github_app_installation_token_for_pull_request(owner, repo) 获取 token，任何已认证用户都可以为任意仓库创建评审会话，造成横向越权。必须在此处添加与 webhook 一致的白名单验证逻辑。
|
|
||
| async def installation_token(self, installation_id: int) -> str: | ||
| payload = await self._request( | ||
| "POST", |
There was a problem hiding this comment.
[Critical] 获取 installation token 时未限制权限。调用 POST /app/installations/{installation_id}/access_tokens 时请求体为空，token 将继承 GitHub App 配置的全部权限（可能包括代码写入、PR合并等）。token 随后被直接通过 GITHUB_TOKEN 环境变量注入沙箱（frontend_sandbox.py:3690），prompt 中的"不要修改仓库"只是 LLM 软约束，没有技术强制力。必须在请求 body 中添加最小权限原则的 permissions：{"pull_requests": "write", "contents": "read", "metadata": "read"}，仅授予评审所需权限。
|
|
||
| # Cache of initialized session paths to avoid re-creating symlinks | ||
| _session_path_cache: dict[str, Path] = {} | ||
| DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions") |
There was a problem hiding this comment.
[Major] DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions") 硬编码了开发者个人主目录路径，在生产环境、其他开发者机器或容器环境中该路径不存在，会导致目录创建失败和功能不可用。应使用跨平台默认路径，如 Path.home() / ".cache" / "veadk" / "sessions" 或 Path(tempfile.gettempdir()) / "veadk-sessions"。（下方注释也同步硬编码了该路径，需要一并修改）
| _safe_error_message(error), | ||
| ) | ||
|
|
||
| asyncio.create_task(_run_review_message()) |
There was a problem hiding this comment.
[Minor] asyncio.create_task(_run_review_message()) 创建的后台任务没有保存引用，服务器关闭时可能出现 "Task was destroyed but it is pending" 警告。另外 _run_review_message 只捕获了 SandboxError，其他异常（网络错误、JSON 解析错误等）会被 asyncio 静默吞掉且不更新 review record 状态。参考第 1045-1047 行的 _background_tasks 模式，保存 task 引用到集合并在 shutdown 时优雅取消；同时添加 except Exception 兜底记录 error 日志并标记 review 状态为 failed。
| raise GitHubAppReviewError( | ||
| "缺少 PyJWT 依赖，无法生成 GitHub App JWT。" | ||
| ) from error | ||
| issued_at = int(time.time()) - 60 |
There was a problem hiding this comment.
[Nit] issued_at = int(time.time()) - 60 硬编码 60 秒时钟偏差补偿。建议提取为常量 JWT_CLOCK_SKEW_SECONDS = 60 并添加注释说明这是为了补偿 GitHub App JWT 的时钟偏差问题，提升可维护性。
There was a problem hiding this comment.
Code Review Findings (HEAD: 6a68651 — 第八轮评审)
本评审针对最新提交 6a68651 ("fix(test): remove release entrypoint shutdown race")。该提交仅修复了 tests/cli/test_studio_release.py 中的测试竞态条件。前七轮指出的 2 个 Critical 安全问题和核心 Major 问题仍未修复。
✅ 本轮已修复/CI状态
- 测试竞态条件：tests/cli/test_studio_release.py shutdown race 已修复
- License-Check: PASS
- pre-commit/gitleaks/codex-smoke/gate: PASS
- build/test (3.10/3.12): PENDING
Critical（未修复，第八轮）
1. 权限绕过：手动 PR 评审端点仍缺少仓库访问控制白名单检查
- 位置：veadk/cli/frontend_sandbox.py:4056-4059 (/web/github/pull-request-reviews POST 端点)
- 问题：对比 webhook 端点（第 3951-3970 行）正确验证了 enabled_repositories() 白名单，但手动评审端点在第 4056 行直接调用 _github_app_installation_token_for_pull_request(owner, repo) 获取 token 并创建沙箱会话，完全跳过了仓库白名单检查和管理员权限校验。
- 影响：横向越权 — 任何已认证用户可传入任意 PR URL（包括 GitHub App 安装下的私有仓库），获取 installation token 并在沙箱中执行任意代码操作，可读取私有仓库代码。
- 修复：在第 4056 行获取 token 前，必须添加与 webhook 端点一致的检查：(1) 验证 store is not None；(2) 验证目标仓库在 enabled_repositories() 白名单中；(3) 推荐额外校验当前用户是否为管理员。
2. GitHub Installation Token 权限仍未受限，原始 token 直接暴露给沙箱
- 位置：veadk/cli/github_app_pr_review.py:614-625 (installation_token 方法) + veadk/cli/frontend_sandbox.py:3690 (通过 GITHUB_TOKEN 环境变量注入沙箱)
- 问题：调用 POST /app/installations/{installation_id}/access_tokens 时未传递 permissions 参数，token 继承 GitHub App 配置的全部权限；原始 token 通过环境变量直接注入沙箱，prompt 中的软约束没有技术强制力。
- 影响：LLM 幻觉、prompt 注入或恶意利用都可能导致仓库代码被修改、PR 被合并等破坏性操作。
- 修复：(1) 在 access_tokens 请求 body 中添加最小权限 permissions: {"pull_requests": "write", "contents": "read", "metadata": "read"}；(2) 推荐封装代理层而不是直接暴露原始 token。
Major（未修复，第八轮）
3. 硬编码开发者个人主目录路径
- 位置：veadk/tools/skills_tools/session_path.py:25
- 问题：DEFAULT_SKILLS_WORK_DIR = Path("/home/gem/veadk_skills/sessions") 是开发者个人目录，在生产环境或其他机器上不存在。
- 影响：Skills 会话路径初始化失败，相关工具不可用。
- 修复：使用 Path.home() / ".cache" / "veadk" / "sessions" 或 tempfile 目录；同步更新测试断言。
Minor（未修复，第八轮）
- 后台任务 asyncio.create_task(_run_review_message()) (frontend_sandbox.py:3769) 未使用已有的 _background_tasks 集合管理，异常捕获不完整。
- JWT iat 偏移量硬编码为 60 秒 (github_app_pr_review.py:739)，建议提取为常量。
- Webhook 自动评审无速率限制，存在资源耗尽风险。
- reference/actb-mono gitlink 残留（无 .gitmodules），建议清理。
Verification
1 participant
No description provided.