Conversation
ctlaltlaltc
force-pushed
the
agent-skill-dev
branch
2 times, most recently
from
55e6482 to
3c7c0ba
Compare
c121914yu
force-pushed
the
agent-skill-dev
branch
from
3c7c0ba to
ca86e68
Compare
ctlaltlaltc
force-pushed
the
agent-skill-dev
branch
from
93d8647 to
69b000a
Compare
|
✅ Build Successful - Preview code-sandbox Image for this PR: |
|
✅ Build Successful - Preview fastgpt Image for this PR: |
|
✅ Build Successful - Preview mcp_server Image for this PR: |
c121914yu
force-pushed
the
agent-skill-dev
branch
3 times, most recently
from
342e62f to
0a4ad20
Compare
|
✅ Docs Preview Deployed! 🔗 👀 Click here to visit preview |
c121914yu
left a comment
c121914yu
left a comment
There was a problem hiding this comment.
Agent Skill Dev — Code Review
感谢提交这个相当完整的 Agent Skill 系统!整体架构清晰:Skill CRUD + 版本管理 + 沙箱编辑/运行时 + 权限继承 + 审计 五个层次都有覆盖,API 合约用 zod schema 定义并接入了 OpenAPI 文档,符合项目规范。
这是一个 +29,704 / -6,255 行 的大 PR,我重点审查了核心服务层(packages/service/core/agentSkills/)、沙箱生命周期(workflow/dispatch/ai/agent/sub/sandbox/)、权限校验和关键 API 路由。受 GitHub diff 行数上限影响,前端 UI、i18n、Helm/Docker 部署相关变更未逐行审查。
⚠️ 必须修复 (🔴 阻塞合并)
1. parseYamlFrontmatter 嵌套对象解析逻辑有 Bug (packages/service/core/agentSkills/utils.ts:47-98)
手写的 YAML parser 使用 stack 但只 push 不 pop,也不跟踪缩进层级。一旦遇到 metadata: 这样的嵌套键,后续所有字段都会被错误地写入嵌套对象,即使它们应该在根级别。
例如:
name: my-skill
metadata:
category: tools
description: My skill # ❌ 会被错误写入 metadata.descriptionSKILL.md frontmatter 解析是导入/部署技能的关键路径,这个 bug 会导致数据静默丢失。强烈建议改用 js-yaml (项目已在其他模块用过)。
2. createSkill 缺少 name 字符集校验 → 沙箱内 shell 命令注入风险 (projects/app/src/pages/api/core/agentSkills/create.ts:67-71)
创建技能时只校验 name 长度 ≤50,没有限制字符集。但是 lifecycle.ts:152,158 把 skill.name 直接拼到 shell 命令里:
const zipPath = `${workDirectory}/package_${skill.name}.zip`;
await sandbox.execute(
`cd ${workDirectory} && unzip -o package_${skill.name}.zip && rm package_${skill.name}.zip`
);如果用户创建一个名字含 ; rm -rf /; # 的技能,部署到沙箱时会执行任意命令。即使沙箱隔离能限制爆炸半径,这仍然是一个真实的注入漏洞。
建议:在 create/update 入口校验 name 必须匹配 /^[a-z0-9][a-z0-9-]{0,49}$/,与 extractSkillFromMarkdown 中的校验保持一致。或者在 lifecycle 中改用 base64/hash 命名 zip 文件。
3. canModifySkill 是死代码,且逻辑过时 (packages/service/core/agentSkills/controller.ts:256-273)
这个函数已经被 authSkill 取代,全仓库只有它自己的定义和一行注释引用。但它的逻辑只检查 tmbId === skill.tmbId(创建者),完全忽略了协作者权限和文件夹继承权限。应该删除,避免后续被人误用。
4. 沙箱数量限制存在竞态条件 (packages/service/core/agentSkills/sandboxController.ts:237-247, lifecycle.ts:272-285)
const activeCount = await MongoSandboxInstance.countDocuments({...});
if (activeCount >= maxEditDebug) throw new Error(...);
// ⚠️ 中间窗口: 多个并发请求都会通过
await newSandbox.create();两个并发请求都会通过 count 检查,然后都创建沙箱,突破上限。应该用 MongoSandboxInstance.findOneAndUpdate 加原子锁,或者用 Redis 分布式锁。在多副本部署下这个问题会更严重。
🟡 建议改进
5. 路径穿越校验不完整 (packages/service/core/agentSkills/archiveUtils.ts:31)
if (!normalized || normalized.includes('../')) continue;这个校验漏掉了:
..单独作为路径(没有/)- 以
/..结尾的路径(foo/..) - 大小写或 URL 编码绕过(取决于
decompress库的解析)
建议:用 path.posix.normalize() 后判断 result.startsWith('..') 或 path.isAbsolute。这些恶意路径最终会被 repackFileMapAsZip 重新打包,然后被沙箱内的 unzip -o 解压(unzip 默认允许 .. 组件,需要 -: 才禁用)。
6. findSkillAndAllChildren / getParents 没有循环检测 (controller.ts:305-344, 411-427)
两个递归函数都没有 visited set,如果数据库里 parentId 形成环(理论上 schema 没有约束),会导致无限递归 → 栈溢出。updateParentFoldersUpdateTime(同文件 432 行)正确地用了 existsId set,这两个函数应该参照实现。
另外 findSkillAndAllChildren 是 N+1 查询模式 — 每层文件夹一个查询。建议改用 MongoDB $graphLookup aggregation。
7. 'edit-debug' 魔法字符串散落在 5+ 处
搜索结果(部分):
sandboxController.ts:127定义了EDIT_DEBUG_CHAT_ID = 'edit-debug'但只在本文件内使用lifecycle.ts:445、debugChat.ts:233、save-deploy.ts:63都重复硬编码了'edit-debug'
应该提到 @fastgpt/global/core/agentSkills/constants 导出共享。
8. fetchSkillsMetaForPrompt 每次 dispatch 都从 S3 下载 ZIP (capability/sandboxSkills.ts:62-120)
仅仅是为了构造 prompt 中的 SKILL.md 路径,就要并发下载所有技能 ZIP 包并解压。这是 hot path(每次 agent 调用都走),应该把 SKILL.md 的相对路径在 import/save-deploy 时存到 version metadata 里,运行时直接用。
9. 项目日志规范违反: 4 处 console.* (schema.ts:113, version/schema.ts:83, save-deploy.ts:187, edit.ts:82)
根据 .claude/skills/system-pr_review/style/logger.md:
服务端统一使用
@fastgpt/service/common/logger,避免console.log
虽然 schema 索引创建在模块加载时,但仍然应该走统一 logger。save-deploy.ts 和 edit.ts 的 catch 块里 console.error 也丢失了上下文(skillId、tmbId、teamId),不利于事后排查。
10. save-deploy.ts / edit.ts / import.ts 没用 NextAPI 包装器
项目里的 API 路由约定是 export default NextAPI(handler)(参见 create.ts)。这三个文件直接 export default async function handler 绕过了统一中间件 — 失去了请求日志、错误处理、链路上下文等能力。SSE 路由可能有特殊处理需求(edit.ts),但 save-deploy.ts 返回 JSON,理论上应该用 NextAPI。
11. existingInstance.metadata!.endpoint! 非空断言不安全 (sandboxController.ts:143,191)
这两个非空断言没有任何前置校验,如果数据库中存在历史数据(metadata 缺 endpoint 字段),会运行时崩溃。建议加显式检查并降级到重新创建沙箱。
12. buildSessionHandler 与 buildEditDebugHandler ~99% 重复 (sandboxSkills.ts:369-485)
两个 handler 几乎完全相同(只有错误处理细节差异),违反 DRY。建议抽取共享的 toolId → schema 映射表,然后用单个 handler factory。
13. isSandboxExpiredError 启发式过宽松 (sandboxSkills.ts:122-136)
return (
msg.includes('not found') ||
msg.includes('not exist') ||
msg.includes('connection') || // ⚠️ 非常宽松
msg.includes('sandbox_not_found') ||
msg.includes('econnrefused') ||
msg.includes('econnreset')
);'connection' 这种字符串可能匹配很多无关错误(比如下游 API 连接错误),会导致不必要的沙箱重建。建议改用错误类型/code 判定,或者从 SDK 暴露专用的 SandboxNotFoundError。
14. 沙箱查询缺少 teamId 隔离 (sandboxController.ts:130-134, lifecycle.ts:193-196,443-447)
const existingInstance = await MongoSandboxInstance.findOne({
appId: skillId,
chatId: EDIT_DEBUG_CHAT_ID,
'metadata.sandboxType': SandboxTypeEnum.editDebug
});虽然外层 API 已经做了 authSkill 校验,但 service 层方法本身没有 teamId scope,纵深防御缺失。如果以后有内部调用绕过 API 层,可能产生跨团队数据访问。
15. updateParentFoldersUpdateTime 用事务做 fire-and-forget (controller.ts:432-452)
这个函数是 fire-and-forget(.catch 吃掉错误),却用了 mongoSessionRun — 事务的开销没有必要。改用 bulkWrite 一次性更新所有父文件夹的 updateTime 即可。
16. import.ts:174 错误响应可能泄漏内部信息
jsonRes(res, { code: 500, error: err });直接传 err 对象,会被序列化包括堆栈、内部路径等。应该用 err?.message || 'Import failed'。
🟢 可选优化
packageSkillInSandbox接受workDirectory参数并直接拼到find命令里。当前所有 caller 都用defaults.workDirectory,但函数签名暴露了风险。建议要么去掉参数,要么校验值是合法的绝对路径。buildVolumeConfig(sandboxConfig.ts:328-340) 把 sessionId 转成 DNS label 时,如果 sessionId 全是特殊字符可能产生空字符串,违反 K8s naming。加一个 fallback。buildBaseContainerEnv把FASTGPT_SESSION_ID暴露到沙箱容器。当sessionId === chatId时,等同于把 chatId 透给用户代码。值得评估隐私影响。existingInstance.metadata?.skillIds.map(String)(lifecycle.ts:221-223) 用了可选链但.map没保护,如果metadata存在但skillIds是 undefined 会崩溃。- YAML parser 还有其他问题:不支持多行字符串、转义引号、嵌套数组、注释末尾;
Number('1e5')会通过数值检测但语义错误。综合看,确实应该换js-yaml。 parseSkillMarkdown的error字段返回了但部分调用方没检查(如lifecycle.ts:116、sandboxSkills.ts:90)— 解析失败时会用空 frontmatter 继续运行,可能掩盖问题。
🧪 建议补充的测试
parseYamlFrontmatter嵌套对象:验证 root-level 字段不会被错误归到 nested 对象extractToFileMap路径穿越:针对..、foo/..、绝对路径(TAR)、Windows\路径的边界用例createEditDebugSandbox并发限流:并发 N+1 个请求,断言只有 N 个成功findSkillAndAllChildren循环 parent:构造环形数据,断言不会栈溢出- 技能名特殊字符:含
;、$、空格、引号的技能名能否走通整个 import → deploy → unzip → run 链路 authSkill权限继承:协作者通过文件夹继承权限的场景
📊 总体评价
| 维度 | 评分 |
|---|---|
| 架构设计 | ⭐⭐⭐⭐☆ — 分层清晰,capability 抽象合理 |
| 安全性 | ⭐⭐⭐☆☆ — 权限继承做得好,但有 shell 注入和路径穿越隐患 |
| 健壮性 | ⭐⭐⭐☆☆ — YAML parser bug + 多个非空断言 + 缺少循环检测 |
| 性能 | ⭐⭐⭐☆☆ — 热路径上重复下载 ZIP 是显著浪费 |
| 代码风格一致性 | ⭐⭐⭐☆☆ — 4 处 console、3 个路由绕过 NextAPI、魔法字符串散落 |
🚀 审查结论
Request changes — 主要是 YAML parser bug(#1)、技能名注入(#2)、死代码(#3)和竞态条件(#4)需要在合并前解决。其他 🟡 项可以分批修复但建议在合并前至少处理 #5、#6、#9。
这是一个体量大、业务复杂的功能,能做到当前完整度已经很不容易。期待看到修复后的版本! 🙏
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🔴 严重 Bug:嵌套对象解析逻辑错误
这个手写 YAML parser 用 stack 但只 push 不 pop,也不跟踪缩进。一旦遇到 metadata: 这样的嵌套键,后续所有字段都会被错误写入 nested 对象,即使是 root-level 字段。
复现:
name: my-skill
metadata:
category: tools
description: My skill实际解析结果:description 会被错误归到 metadata.description,而不是 root。
SKILL.md frontmatter 解析是导入/部署技能的核心路径,这个 bug 会导致数据静默丢失。
强烈建议改用 js-yaml(npm 上最成熟的 YAML 库),手写 parser 还有其他若干局限(多行字符串、转义、注释末尾、1e5 数值误判等)。
import yaml from 'js-yaml';
function parseYamlFrontmatter(text: string): Record<string, any> {
const result = yaml.load(text);
return typeof result === 'object' && result !== null ? (result as Record<string, any>) : {};
}| onProgress?.({ sandboxId, phase: 'extractingPackage', skillName: skill.name }); | ||
| const extractResult = await sandbox.execute( | ||
| `cd ${workDirectory} && unzip -o package_${skill.name}.zip && rm package_${skill.name}.zip` | ||
| ); |
There was a problem hiding this comment.
🔴 沙箱内 shell 命令注入风险
skill.name 被直接拼接到 shell 命令里:
`cd ${workDirectory} && unzip -o package_${skill.name}.zip && rm package_${skill.name}.zip`而 projects/app/src/pages/api/core/agentSkills/create.ts:67-71 在创建技能时没有校验 name 字符集 — 只校验长度 ≤50。这意味着用户可以创建一个名为 ; rm -rf /; # 的技能,部署时会在沙箱内执行任意命令。
虽然沙箱隔离能限制爆炸半径,但这是一个真实的注入漏洞,而且也违反纵深防御原则。
两种修复方式(任选其一):
- 在
create.ts和update.ts的入口校验 name 字符集,与extractSkillFromMarkdown中的正则保持一致:
if (!/^[a-z0-9][a-z0-9-]{0,49}$/.test(name.trim())) {
throw new Error('Skill name must be lowercase alphanumeric and hyphens only');
}- 在 lifecycle 中改用 hash 或 skillId 命名 zip 文件,完全避开用户输入:
const zipPath = `${workDirectory}/package_${skill._id}.zip`;|
|
||
| // Only the creator can modify | ||
| return skill.tmbId?.toString() === tmbId; | ||
| } |
There was a problem hiding this comment.
🔴 死代码 + 逻辑过时
全仓库搜索 canModifySkill:只有这里的定义,以及 save-deploy.ts:51 的一行注释引用。它已经被 authSkill 取代。
更糟的是它的逻辑只检查 tmbId === skill.tmbId(只允许创建者),完全忽略了:
- 团队所有者
- 协作者写权限
- 文件夹继承权限
如果未来有人误用这个函数,会绕过整个权限系统。建议直接删除。
| onProgress?.({ sandboxId: skillId, phase: 'failed', message }); | ||
| throw new Error(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 竞态条件:沙箱数量限制非原子
const activeCount = await MongoSandboxInstance.countDocuments({...});
if (activeCount >= maxEditDebug) throw new Error(...);
// ⚠️ 中间窗口: N 个并发请求都会通过
await newSandbox.create();并发场景:N 个用户同时点 "打开调试沙箱",N 个请求都看到 activeCount = limit - 1,N 个请求都通过检查,N 个沙箱被创建 → 突破上限。在多副本部署下问题更严重。
修复方案:
- 用 Redis 分布式锁 (
SETNX+ TTL) - 或者先
findOneAndUpdate一个 "counter" 文档,失败则拒绝 - 或者直接在
MongoSandboxInstance.create后立刻countDocuments,如果超过 limit 就回滚刚创建的实例
lifecycle.ts:272-285 有同样的问题,需要一并修。
| fileMap[normalized] = file.data; | ||
| } | ||
| return fileMap; | ||
| } |
There was a problem hiding this comment.
🟡 路径穿越校验不完整
if (!normalized || normalized.includes('../')) continue;漏掉的情况:
..单独(没有/)- 以
/..结尾(foo/..) - TAR 文件中的绝对路径(经过
replace(/^\/+/, '')后变成相对路径,但仍然能把文件写到 workspace 根目录)
这些恶意路径最终会被 repackFileMapAsZip 重新打包,然后被沙箱内的 unzip -o 解压。unzip 默认允许 .. 路径组件(需要 -: 才禁用)。
建议改用 path.posix.normalize 验证:
import path from 'path';
const normalized = path.posix.normalize(
file.path.replace(/\\/g, '/').replace(/^\/+/, '')
);
if (!normalized || normalized === '..' || normalized.startsWith('../') || path.posix.isAbsolute(normalized)) {
continue;
}| directory: '' | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 热路径上重复下载 ZIP,显著的性能问题
这个函数在每次 agent dispatch 时都被调用,会并发下载所有技能 ZIP 包(从 S3),解压,只为了构造 prompt 中的 SKILL.md 路径。
如果一个 agent 配置了 5 个技能,每次对话都要 5 次 S3 GET + 5 次 ZIP 解压。
建议:在 import / save-deploy 时把 SKILL.md 的相对路径(info.relativePath)和 frontmatter 解析结果直接存到 MongoAgentSkillsVersion.metadata 中,运行时直接读 DB。
// version schema 增加字段
skillMdRelativePath: { type: String },
frontmatter: { type: Object } // 缓存 name/description/avatar这样 fetchSkillsMetaForPrompt 就可以缩减成一次 MongoDB 查询。
| ); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 错误识别启发式过于宽松
return (
msg.includes('not found') ||
msg.includes('not exist') ||
msg.includes('connection') || // 太宽松
msg.includes('sandbox_not_found') ||
msg.includes('econnrefused') ||
msg.includes('econnreset')
);'connection' 这种通用词可能匹配下游 API、数据库甚至 LLM 提供商的连接错误,会触发不必要的沙箱重建,放大故障。
建议:从 @fastgpt-sdk/sandbox-adapter 暴露专用 error class(如 SandboxNotFoundError / SandboxUnreachableError),用 instanceof 判定,而不是字符串匹配。
| sandboxId: existingInstance.sandboxId | ||
| }); | ||
|
|
||
| const endpointInfo = existingInstance.metadata!.endpoint!; |
There was a problem hiding this comment.
🟡 非空断言不安全
const endpointInfo = existingInstance.metadata!.endpoint!;这两个非空断言没有任何前置校验。如果数据库里有历史数据(metadata 缺 endpoint 字段,或者旧版字段名不一致),会直接抛 Cannot read properties of undefined。
建议显式检查并降级到重新创建沙箱:
const endpointInfo = existingInstance.metadata?.endpoint;
if (!endpointInfo) {
addLog.warn('[Sandbox] Running instance missing endpoint, recreating', { instanceId: existingInstance._id });
// fall through to creation path
} else {
// reuse path
}|
|
||
| res.end(); | ||
| } catch (err: any) { | ||
| console.error('[API] Create edit-debug sandbox error:', err); |
There was a problem hiding this comment.
🟡 违反项目日志规范
根据 .claude/skills/system-pr_review/style/logger.md:服务端统一使用 @fastgpt/service/common/logger,避免 console.log/console.error。
这里 console.error 还丢失了关键上下文(skillId、tmbId、teamId),不利于事后排查。
建议:
import { getLogger, LogCategories } from '@fastgpt/service/common/logger';
const logger = getLogger(LogCategories.MODULE.AGENT_SKILLS.SANDBOX);
// catch 中:
logger.error('Create edit-debug sandbox failed', { error: err, skillId, tmbId, teamId });同样问题: save-deploy.ts:187、schema.ts:113、version/schema.ts:83。
| const handler = handlers[toolId]; | ||
| if (!handler) return { response: 'Unknown sandbox tool', usages: [] }; | ||
| return handler(); | ||
| } |
There was a problem hiding this comment.
🟡 buildEditDebugHandler 与 buildSessionHandler 99% 重复
两个 handler 几乎一模一样,只在 dispose/renew 细节上不同。违反 DRY,后续维护要同步改两处容易出错。
建议:抽取一个 toolId → schema 映射表,然后统一一个 handler factory:
const SANDBOX_TOOL_DISPATCH = {
[SandboxToolIds.readFile]: { schema: SandboxReadFileSchema, dispatch: dispatchSandboxReadFile, withSkillRef: true },
[SandboxToolIds.writeFile]: { schema: SandboxWriteFileSchema, dispatch: dispatchSandboxWriteFile },
// ...
} as const;
async function dispatchSandboxTool(toolId: string, args: string, ctx, ...) {
const def = SANDBOX_TOOL_DISPATCH[toolId];
if (!def) return null;
const parsed = def.schema.safeParse(parseJsonArgs(args));
if (!parsed.success) return { response: parsed.error.message, usages: [] };
// ...
}
…figs (#6710) * chore: Rename container names for consistency in Docker configs * chore: Rename service names for consistency in Docker configs chore: Update OpenSandbox versions and image repositories (#6709) * chore: Update OpenSandbox versions and image repositories * yml version * images * init yml * port --------- Co-authored-by: archer <545436317@qq.com> refactor(chat): optimize sandbox status logic and decouple UI/Status hooks (#6713) * refactor(chat): optimize sandbox status logic and decouple UI/Status hooks * fix: useRef, rename onClose to afterClose Update .env.template (#6720) aiproxy默认的请求地址改成http协议 feat: comprehensive agent skill management and sandbox infrastructure optimization - Skill System: Implemented a full skill management module including CRUD operations, folder organization, AI-driven skill generation, and versioning (switch/update). - Sandbox Infrastructure: Introduced 'volume-manager' for PVC and Docker volume lifecycle management, replacing the MinIO sync-agent for better data persistence. - Workflow Integration: Enhanced the Agent node to support skill selection and configuration, including new UI components and data normalization. - Permission Management: Added granular permission controls for skills, supporting collaborators, owner transfers, and permission inheritance. - UI/UX: Added a dedicated Skill dashboard, sandbox debug interface (terminal, logs, and iframe proxy), and comprehensive i18n support. - Maintenance: Migrated Docker services to named volumes, optimized sandbox instance limits, and improved error handling for sandbox providers. Co-authored-by: chanzhi82020 <chenzhi@sangfor.com.cn> Co-authored-by: lavine77 Signed-off-by: Jon <ljp@sangfor.com.cn> feat: hide skill prettier
c121914yu
force-pushed
the
agent-skill-dev
branch
from
84950e1 to
f8e37ce
Compare
2 participants
No description provided.