feat(web-shell): add skill management pages - #7018
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: This addresses a real UX gap — Web Shell previously rendered Direction: Aligned. Skill management in Web Shell follows the established MCP management UI pattern (search, filter, detail, toggle, install, delete). The feature fills a clear gap in the product's management surface. No CHANGELOG reference needed — this is new functionality. Size: This is a large feature PR (~2,753 production logic lines, ~1,088 test lines, ~86 design doc lines). Not a refactor — the Tier 1 hard-block does not apply. The 1000+ large-PR advisory applies, but the scope is justified: daemon-side validation + routes + SDK bindings + full Web Shell UI is the minimum set needed for a complete management experience. No core paths ( Approach: The scope feels right for the stated goal. The three-layer architecture (daemon validation → SDK bindings → Web Shell UI) follows existing patterns. The security model is thorough — path traversal protection, symlink checks, ZIP bomb limits, staged replacement with rollback, trusted-workspace gating on mutation routes. Feature detection via Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 解决了一个真实的 UX 缺陷——Web Shell 之前将 方向： 对齐。Web Shell 中的技能管理遵循已有的 MCP 管理 UI 模式（搜索、筛选、详情、切换、安装、删除）。该功能填补了产品管理界面的明确空白。无需 CHANGELOG 参考——这是新功能。 规模： 这是一个大型功能 PR（约 2,753 行生产逻辑、约 1,088 行测试、约 86 行设计文档）。不是重构——Tier 1 硬阻断不适用。1000+ 大 PR 建议适用，但范围合理：daemon 端验证 + 路由 + SDK 绑定 + 完整 Web Shell UI 是完整管理体验所需的最小集合。未触及核心路径。 方案： 范围与目标匹配。三层架构（daemon 验证 → SDK 绑定 → Web Shell UI）遵循现有模式。安全模型完善——路径遍历保护、符号链接检查、ZIP 炸弹限制、带暂存回滚的替换、修改路由的信任工作区门控。通过 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "workspace_skill_manage" |
— Qwen Code · serve A/B
Code ReviewRead the full diff against an independent proposal. Before looking at the code, my approach would have been: add daemon routes (install, delete, toggle) with filesystem validation (path traversal, symlink, ZIP bomb limits), staged replacement with rollback, SDK client methods, Web Shell UI with filter/search/detail/toggle/install/delete following the MCP management page pattern, i18n for both languages, capability registration for feature detection, and post-mutation skill status refresh. The PR's approach matches and exceeds this proposal. The security model in The facade correctly invalidates cached skill status ( No critical blockers found. The implementation is careful and well-tested. Reuse check: the PR reuses existing patterns throughout — the MCP management UI components (Card, Badge, Breadcrumb, Empty, AlertDialog), existing workspace service facade architecture, existing SDK Real-Scenario TestingBuilt the full bundle from the PR branch ( Daemon startup and route registration (unauthenticated)Capabilities —
|
| Package | Test file | Tests |
|---|---|---|
| cli | workspace-skill-management.test.ts | 22/22 ✅ |
| cli | workspace-skills.test.ts | 5/5 ✅ |
| cli | server.test.ts | 745/745 ✅ |
| cli | facade.test.ts | 81/81 ✅ |
| sdk-typescript | DaemonClient.test.ts | 275/275 ✅ |
| web-shell | skills-manager-logic.test.ts | 3/3 ✅ |
| web-shell | App.test.tsx | 111/111 ✅ |
| webui | DaemonWorkspaceProvider.test.tsx | 16/16 ✅ |
Not tested
Web Shell browser E2E — requires an authenticated daemon session with a browser. The PR author also noted this limitation. The screenshots in the PR body show the UI working correctly on macOS.
中文说明
代码审查
阅读完整 diff 并与独立方案对比。PR 的方案匹配并超越了独立提案。安全模型尤其完善——workspace-skill-management.ts（881 行）处理了所有边缘情况：各层符号链接检查、ZIP 条目类型验证（通过 externalFileAttributes 拒绝符号链接，通过 generalPurposeBitFlag 拒绝加密条目）、文件数/大小/深度限制、路径规范化、重复路径检测、GitHub 下载 URL 验证（仅允许 raw.githubusercontent.com 或 github.com HTTPS）、带原子重命名和回滚的暂存目录替换、以及 SKILL.md 名称匹配。GitHub 下载有优雅的降级路径：先 API，API 非 404 失败时回退到 git clone --sparse。
facade 在修改后正确使缓存状态失效并刷新活跃的 ACP 会话。切换路由正确检查未激活扩展——扩展级别禁用的技能无法单独重新启用。
未发现关键阻断问题。 实现谨慎且经过充分测试。
实际场景测试
从 PR 分支构建完整 bundle 并在 tmux 中对未认证和已认证配置运行 daemon。
- Daemon 启动正常，
workspace_skill_manage能力已注册 GET /workspace/skills返回完整技能列表- 修改路由（install/delete/toggle）正确要求认证（401），返回带 code 的结构化错误
- 已认证模式下，验证失败返回正确的结构化错误（invalid_skill_name, invalid_skill_scope）
- 所有 1,258 个单元测试通过
未测试：Web Shell 浏览器 E2E——需要已认证的 daemon 会话。
— Qwen Code · qwen3.7-max
Reviewed at 41a5a080692c05242ef496650d35174f57f5a847 · re-run with @qwen-code /triage
|
Confidence: 4/5 — Solid feature PR; well-engineered Skill management surface that fills a real UX gap, with thorough security validation and comprehensive test coverage. This is a genuinely good feature PR. The Skill management page is something Web Shell clearly needed — the previous experience of rendering The daemon-side implementation in The Web Shell UI follows the established MCP management page pattern (Card grid, filter/search, detail view, toggle, install dialog, delete confirmation). The i18n coverage is complete for both English and Chinese, with proper error code → user-facing message mapping. The skill filter logic is cleanly separated into All 1,258 unit tests pass across every affected package. The daemon starts cleanly, registers all three skill management capabilities ( The PR is large (~2,753 production lines), but every piece is needed for the complete management experience — daemon validation, routes, SDK bindings, Web Shell UI, and i18n. No unrelated changes or drive-by refactors. No blocking concerns. Approving. 中文说明信心度：4/5 — 扎实的功能 PR；完善的技能管理界面填补了真实的 UX 空白，安全验证全面，测试覆盖充分。 这是一个真正优秀的功能 PR。Web Shell 显然需要技能管理页面——之前将 daemon 端实现（ Web Shell UI 遵循已有的 MCP 管理页面模式。i18n 覆盖完整。技能筛选逻辑干净地分离到 PR 较大（约 2,753 行生产代码），但每一部分都是完整管理体验所必需的。无阻断问题。批准。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Security & Correctness Review — PR #7018
Reviewed workspace-skill-management.ts (+796), workspace-skills.ts routes (+201), workspace-service/index.ts facade, error-response.ts, and tests. Overall the backend is well-engineered — the security model is layered and the attack surfaces (ZIP extraction, GitHub download, folder traversal) are each bounded with multiple defenses. Findings below are Minor/Nit; nothing rises to Critical or Major.
Security posture — what's done well
The code gets the hard parts right:
- Path traversal:
normalizeRelativePathrejects.., absolute paths, empty segments, depth > 16, length > 512. - ZIP symlink attacks: Detected via
externalFileAttributes >>> 16 & 0xf000 === 0xa000. - Folder symlink attacks: Each child entry is checked with both
Dirent.isSymbolicLink()and a secondarylstatverification. - Size bounds:
MAX_FILES=128,MAX_FILE_BYTES=2MB,MAX_TOTAL_BYTES=6MBenforced at extraction, download, and normalization layers. - Staged install with rollback: Files are written to a temp directory, SKILL.md is validated, then an atomic
renamecommits. - Delete authorization: The facade resolves
installedPathfrom the live skill listing (not from user input), thendeleteWorkspaceSkillverifies the path is within an allowed base directory, has no symlinks, and the SKILL.md name matches. - Shell injection:
execFile(notexec) for git operations;owner/repovalidated against[A-Za-z0-9._-]+. - HTTPS-only for GitHub URLs; download URLs verified to be
raw.githubusercontent.com. - Encrypted ZIP entries rejected. Prototype pollution defense via
safeBody. - JSON body limit: 10mb in
installJsonBodyParser, sufficient for ~8MB base64 payload.
Minor
M1. ref (branch/tag) not validated in GitHub URL parsing
packages/cli/src/serve/workspace-skill-management.ts, downloadGitHubSkill()
owner and repo are validated against /^[A-Za-z0-9._-]+$/, but ref is passed unvalidated to git clone --branch <ref> in the fallback path. While execFile prevents shell injection, a crafted ref starting with -- could potentially be interpreted as an additional git flag. Low practical risk (git's --branch consumes the next argument as its value), but adding the same regex validation would be defense-in-depth:
if (!owner || !repo || !ref ||
!/^[A-Za-z0-9._-]+$/.test(owner) ||
!/^[A-Za-z0-9._-]+$/.test(repo) ||
!/^[A-Za-z0-9._/+-]+$/.test(ref)) { // ← add ref validationM2. fetchBytes reads entire response body before size check
packages/cli/src/serve/workspace-skill-management.ts, fetchBytes()
The Content-Length header is checked as a fast-path, but response.arrayBuffer() reads the full body into memory before the post-read check. A server that omits or lies about Content-Length can force the process to allocate up to the body size. Each file is capped at 2MB post-read, but 128 files × 2MB = 256MB of allocations before rejection. Consider a streaming approach with an early abort:
// Instead of:
const content = Buffer.from(await response.arrayBuffer());
// Consider reading with a size-bounded stream that destroys on overflow.M3. Rollback removes successful install on backup cleanup failure
packages/cli/src/serve/workspace-skill-management.ts, installWorkspaceSkill()
After fs.rename(staging, destination) commits the new skill, fs.rm(backup) is inside the try block. If it throws (unlikely with force: true, but possible on NFS or permission edge cases), the catch block runs fs.rm(destination) (removing the just-installed skill) and fs.rename(backup, destination) (attempting rollback). A successful install is lost due to a cleanup failure. Consider moving backup removal outside the try/catch:
await fs.rename(staging, destination);
// Install committed — backup cleanup failure should not trigger rollback
if (movedExisting) {
await fs.rm(backup, { recursive: true, force: true }).catch(() => {});
}M4. Delete route lacks name-length and format validation at route layer
packages/cli/src/serve/routes/workspace-skills.ts, DELETE /workspace/skills/:name
The install route validates name length (parseSkillInstallRequest checks MAX_WORKSPACE_SKILL_NAME_LENGTH) and the toggle route does the same (parseSkillToggleRequest), but the delete route passes req.params['name'] directly to the facade without route-level validation. The facade does a case-insensitive lookup, so an oversized or malformed name just results in a "skill not found" error. No security impact, but inconsistent with sibling routes and misses an opportunity for a clear 400 before touching the service layer.
M5. Root folder path not checked for being a symlink
packages/cli/src/serve/workspace-skill-management.ts, filesFromFolder()
fs.realpath(folderPath) resolves the root (following symlinks), but the original folderPath is never checked for being a symlink itself. Children are rigorously checked with both Dirent.isSymbolicLink() and lstat. For the trusted-workspace trust model this is low risk — the user providing the path is already authorized — but it's an inconsistency with the child-entry checks. A simple lstat check on the original path before realpath would close the gap.
Nits
N1. downloadGitHubDirectory recalculates total bytes with O(n²) reduce
After each file download, the total is recalculated from scratch: files.reduce((total, file) => total + file.content.length, 0). With 128 files that's ~8K iterations — not a real performance issue, but a simple accumulator variable (totalBytes += content.length) would be cleaner and consistent with how filesFromZip and filesFromFolder track totals.
N2. ZIP totalBytes uses declared uncompressedSize for running total
filesFromZip accumulates entry.uncompressedSize from ZIP headers, which a malicious ZIP could falsify. However, readZipEntry independently checks actual bytes read against MAX_FILE_BYTES, so actual memory usage is bounded at MAX_FILES × MAX_FILE_BYTES. The defense-in-depth is adequate; tracking actual bytes read would be slightly more robust.
N3. No test coverage for downloadGitHubDirectoryWithGit fallback
The tests mock fetch for the API-based download path but don't exercise the git clone fallback (downloadGitHubDirectoryWithGit). This path has the most complex external interaction (sparse checkout, temp directory lifecycle, 60s timeout). Consider adding a test that stubs child_process.execFile or triggers the API-failure fallback.
N4. downloadGitHubDirectoryWithGit constructs --branch from URL-decoded ref
The ref passes through decodeURIComponent (via the URL segment split) before being used as --branch <ref>. While execFile prevents shell injection, URL-encoded control characters in ref would be decoded and passed to git. Combined with M1's recommendation to validate ref, this is fully mitigated.
Summary
| Severity | Count | Action needed |
|---|---|---|
| Critical | 0 | — |
| Major | 0 | — |
| Minor | 5 | M3 (rollback) and M2 (streaming fetch) are the most impactful |
| Nit | 4 | Optional cleanup |
The PR is in good shape for merge. M3 (rollback on cleanup failure) is the most actionable item — it could cause data loss in an edge case where the install actually succeeded. The other findings are defense-in-depth improvements.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #7018 — feat(web-shell): add skill management pages
Type: New Feature
Change size: +3491/-75 across 37 files
HEAD: bca53e8a
Findings Summary
- Critical/Major: 0
- Minor: 5
- Nit: 4
Review
Comprehensive skill management feature with solid backend security. Layered defenses across all attack surfaces: input validation, path traversal protection, ZIP extraction safety, staged replacement with rollback, and size/count/depth limits on GitHub downloads.
Minor Findings
-
Rollback removes successful install on backup cleanup failure: After
fs.rename(staging, destination)commits the new skill,fs.rm(backup)is inside thetryblock. If backup cleanup throws, the catch block removes the just-installed skill — data loss from a cleanup failure. Fix: move backup cleanup outside the try/catch with.catch(() => {}). -
refnot validated in GitHub URLs:ownerandrepovalidated against[A-Za-z0-9._-]+butref(branch/tag) is unvalidated beforegit clone --branch. Low practical risk withexecFilebut easy defense-in-depth. -
fetchBytesreads entire body before size check:response.arrayBuffer()loads full response into memory before post-read size validation. A server lying aboutContent-Lengthcan force up to 256MB of allocations (128 files × 2MB). Streaming approach with early abort would be more robust. -
Delete route lacks route-level name validation: Install and toggle routes validate name length/format; delete passes
req.params['name']directly to facade. No security impact (facade does lookup) but inconsistent. -
Root folder path not checked for being a symlink:
filesFromFolderresolves root viarealpathbut never checks if the original path was a symlink. Children are rigorously checked. Low risk under trusted-workspace model.
Nits
- O(n²) total-bytes reduce in
downloadGitHubDirectory(use accumulator) - ZIP
totalBytesuses declareduncompressedSizerather than actual bytes read - No test coverage for git-clone fallback path
- URL-decoded
refpassed to--branchwithout sanitization
Final Verdict
APPROVE. The skill management feature is well-designed with solid backend security. M1 (rollback-on-cleanup-failure) is the most actionable item — it could cause skill data loss in a rare edge case.
This review was generated by QoderWork AI
|
@qwen-code /triage |
|
Re-run verification on Ran a full verification pass against the current head:
The stage comments couldn't be patched in place this run (token scope) — leaving them untouched rather than reposting duplicates. Score and verdict unchanged: 4/5, LGTM. 中文说明在 对当前 head 进行了完整重新验证：
本次无法就地更新阶段评论（token 权限限制）——保留原有评论而不是重复发布。评分和结论不变：4/5，可以合并。 — Qwen Code · qwen3.7-max Reviewed at |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Not reviewed: chunk 3, chunk 4, chunk 7, chunk 8, chunk 10, chunk 12, chunk 13 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qmodel via Qwen Code /review
| getProjectRoot: () => workspace, | ||
| } as Config) | ||
| .getSkillsBaseDirs(scope === 'workspace' ? 'project' : 'user') | ||
| .map((directory) => path.resolve(directory)); |
There was a problem hiding this comment.
[Suggestion] Concurrent installs of the same skill have no mutual exclusion — installWorkspaceSkill uses a staging→rename→backup pattern with no per-skill lock. Two concurrent requests both pass the fs.lstat(destination) check, both proceed to staging, and the second fs.rename(staging, destination) either fails with ENOTEMPTY (Linux) or silently replaces the first install (macOS). On macOS, the first install is destroyed with no rollback.
Failure scenario: user double-clicks Upload in SkillInstallDialog. R1 completes install, R2's rename replaces R1's directory. R2 returns 200, but R1's install is silently gone.
| .map((directory) => path.resolve(directory)); | |
| // Consider adding a per-skill mutex (e.g. Map<string, Promise> keyed on `${scope}:${skillName}`) | |
| // in the facade to serialize concurrent installs/deletes for the same skill. | |
| await fs.rename(staging, destination); |
— qmodel via Qwen Code /review
| await refreshWorkspaceSkillsAfterMutation(); | ||
| return result; | ||
| }, | ||
|
|
||
| async deleteWorkspaceSkill( |
There was a problem hiding this comment.
[Suggestion] Missing publishWorkspaceEvent after install/delete mutations — toggle publishes settings_changed SSE so connected clients update immediately, but install/delete only invalidate caches. Other browser tabs or connected clients won't see the new/removed skill until the next full status poll.
Failure scenario: user installs a skill via the management page. A second browser tab (or another connected client) doesn't see the new skill until manual refresh.
| await refreshWorkspaceSkillsAfterMutation(); | |
| return result; | |
| }, | |
| async deleteWorkspaceSkill( | |
| await refreshWorkspaceSkillsAfterMutation(); | |
| publishWorkspaceEvent({ | |
| type: 'settings_changed', | |
| data: { key: 'skills.installed', scope: request.scope }, | |
| originatorClientId: _ctx.originatorClientId, | |
| }); | |
| return result; | |
| }, | |
| async deleteWorkspaceSkill( |
— qmodel via Qwen Code /review
| function readZipEntry(zipFile: ZipFile, entry: Entry): Promise<Buffer> { | ||
| return new Promise((resolve, reject) => { | ||
| zipFile.openReadStream(entry, (error, stream) => { | ||
| if (error) { |
There was a problem hiding this comment.
[Suggestion] git clone fallback doesn't receive githubToken for private repos — when the API-based download fails with 401/403, the catch block falls through to this function which runs git clone against https://github.com/${owner}/${repo}.git without credentials. The fallback always fails for private repos, and the original actionable error ("GitHub authentication failed; check GH_TOKEN") is replaced by a generic "Failed to download GitHub Skill".
Failure scenario: user tries to install a skill from a private repo. API returns 401, fallback strips the token, user gets a confusing generic error.
Suggested fix: pass githubToken to the fallback and embed it in the clone URL (e.g. https://x-access-token:${githubToken}@github.com/...), or skip the git fallback when the original error is 401/403.
— qmodel via Qwen Code /review
| scope: DaemonSkillScope, | ||
| ): Promise<DaemonSkillMutationResult> { | ||
| return this.jsonRequest( | ||
| `/workspace/skills/${urlEncode(skillName)}?scope=${scope}`, |
There was a problem hiding this comment.
[Suggestion] scope not URL-encoded in query string — skillName is correctly encoded via urlEncode() but scope is interpolated raw. Other endpoints in the same file (e.g. agent deletion) correctly encode scope. Today DaemonSkillScope is 'workspace' | 'global' (safe ASCII), so no immediate bug — but this inconsistency invites future regression if the union is extended.
| `/workspace/skills/${urlEncode(skillName)}?scope=${scope}`, | |
| `/workspace/skills/${urlEncode(skillName)}?scope=${urlEncode(scope)}`, |
— qmodel via Qwen Code /review
| useEffect(() => { | ||
| embedded?.onDetailChange(Boolean(selectedSkill)); | ||
| }, [embedded, selectedSkill]); | ||
|
|
There was a problem hiding this comment.
[Suggestion] Toggle handler: reload failure after successful toggle shows misleading error — same pattern as the existing inline comment on the install handler at :290. If setEnabled succeeds but the subsequent reload() throws, the catch block reports a toggle error even though the toggle itself succeeded. The status override is already applied, so the UI shows both the correct toggled state AND an error alert.
Suggested fix: wrap reload() in its own try/catch so that a reload failure does not mask a successful toggle.
— qmodel via Qwen Code /review
| const skillFile = path.join(staging, 'SKILL.md'); | ||
| let parsed: ReturnType<SkillManager['parseSkillContent']>; | ||
| try { | ||
| parsed = new SkillManager({} as Config).parseSkillContent( |
There was a problem hiding this comment.
[Suggestion] SkillManager is constructed with {} as Config to call parseSkillContent. This works only because the method does not currently access this.config. If a future change to parseSkillContent or any method it calls reads a Config property, the code crashes at runtime while TypeScript remains satisfied by the as Config assertion.
Concrete cost: fragile cross-package coupling — a one-line change in SkillManager silently breaks workspace skill installation.
— qwen3.7-max via Qwen Code /review
| } catch { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] reloadLoadedSkills sets loadedSkillsReady = true only on the success path. If the initial load fails, loadedSkillsReady stays false indefinitely and the commands memo silently falls back to stale connection.commands for slash-completion, with no user-visible indication that the skill list failed to load.
Concrete cost: stale skill commands persist in autocomplete after a failed reload, self-healing only on retry.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: chunk 2 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 4 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 13 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 13 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 12 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 6 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 8 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 1 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 7 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 10 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 11 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 5 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 3 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: chunk 9 — no prompt was built for it (agent-prompt never ran for this chunk). Not reviewed: Agent 0: Issue fidelity & root-cause ownership — no prompt was built for it (agent-prompt --role 0 never ran). Not reviewed: chunk 1 — no prompt was built for it (agent-prompt --chunk 1 never ran). Not reviewed: chunk 2 — no prompt was built for it (agent-prompt --chunk 2 never ran). Not reviewed: chunk 3 — no prompt was built for it (agent-prompt --chunk 3 never ran). Not reviewed: chunk 4 — no prompt was built for it (agent-prompt --chunk 4 never ran). Not reviewed: chunk 5 — no prompt was built for it (agent-prompt --chunk 5 never ran). Not reviewed: chunk 6 — no prompt was built for it (agent-prompt --chunk 6 never ran). Not reviewed: chunk 7 — no prompt was built for it (agent-prompt --chunk 7 never ran). Not reviewed: chunk 8 — no prompt was built for it (agent-prompt --chunk 8 never ran). Not reviewed: chunk 9 — no prompt was built for it (agent-prompt --chunk 9 never ran). Not reviewed: chunk 10 — no prompt was built for it (agent-prompt --chunk 10 never ran). Not reviewed: chunk 11 — no prompt was built for it (agent-prompt --chunk 11 never ran). Not reviewed: chunk 12 — no prompt was built for it (agent-prompt --chunk 12 never ran). Not reviewed: chunk 13 — no prompt was built for it (agent-prompt --chunk 13 never ran). Not reviewed: Test coverage matrix (whole-diff) — no prompt was built for it (agent-prompt --role test-matrix never ran). Not reviewed: Agent 1b: Removed-behavior audit — no prompt was built for it (agent-prompt --role 1b never ran). Not reviewed: Agent 1c: Cross-file tracer — no prompt was built for it (agent-prompt --role 1c never ran). Not reviewed: Agent 7: Build & test verification — no prompt was built for it (agent-prompt --role 7 never ran). Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped). Not reviewed: verification — the review posts findings, but no verifier ran (Step 4 builds its prompt with agent-prompt --role verify; none was recorded, so the findings were not verified).
— qwen3.7-max via Qwen Code /review
| ? 'skill_inactive_extension' | ||
| : 'skill_not_toggleable', |
There was a problem hiding this comment.
[Suggestion] The new skill_inactive_extension error code is not documented in docs/developers/qwen-serve-protocol.md. The protocol doc still lists code: 'skill_not_toggleable' with reason: 'inactive_extension', but the server now returns code: 'skill_inactive_extension' for that case. SDK clients or third-party integrations reading the docs would not know to handle the new code.
Concrete cost: an external client checking only code === 'skill_not_toggleable' misses the inactive-extension case, showing a generic error instead of the specific "enable the extension first" message.
Also update docs/developers/qwen-serve-protocol.md to document the two-code split.
— qwen3.7-max via Qwen Code /review
| 'plugins.mcpLoadFailed': 'MCP 状态加载失败', | ||
| 'plugins.sections': '插件分类', | ||
| 'plugins.skills': 'Skills', | ||
| 'plugins.skills': '技能', |
There was a problem hiding this comment.
[Suggestion] ZH locale inconsistency: plugins.skills is translated to '技能' but skills.title (line 3761) remains 'Skills' (untranslated English). These two keys label the same concept in different UI locations (plugin sidebar tab vs. skills panel title), so a Chinese-speaking user sees the translated term in one place and the English term in another.
Concrete cost: inconsistent UI labels confuse users navigating between the plugins sidebar and the skills detail panel.
Either translate both keys to '技能' or keep both as 'Skills' for consistency within the ZH locale.
— qwen3.7-max via Qwen Code /review
| async installWorkspaceSkill( | ||
| _ctx: WorkspaceRequestContext, | ||
| request: WorkspaceSkillInstallRequest, | ||
| ): Promise<WorkspaceSkillMutationResult> { |
There was a problem hiding this comment.
[Suggestion] The facade's installWorkspaceSkill and deleteWorkspaceSkill methods have no facade-level tests in facade.test.ts. These methods are not trivial wrappers: installWorkspaceSkill extracts GH_TOKEN/GITHUB_TOKEN from skillInstallEnv and calls refreshWorkspaceSkillsAfterMutation after success. deleteWorkspaceSkill performs case-insensitive skill lookup, validates level/scope mapping, and throws WorkspaceSkillNotFoundError for unknown skills.
Concrete cost: a regression in token extraction, scope/level mapping, or refresh-after-mutation would go undetected.
Suggested fix: add describe('installWorkspaceSkill') and describe('deleteWorkspaceSkill') blocks mirroring the existing setWorkspaceSkillEnabled test pattern.
— qwen3.7-max via Qwen Code /review
| async function downloadGitHubDirectoryWithGit( | ||
| owner: string, | ||
| repo: string, | ||
| ref: string, | ||
| directory: string, | ||
| ): Promise<SkillPackageFile[]> { |
There was a problem hiding this comment.
[Suggestion] The git-clone fallback path (downloadGitHubDirectoryWithGit) has no test coverage. When the GitHub API returns a non-404 github_api_failed error, the code falls back to git clone --depth 1 --filter=blob:none --sparse. No test covers this fallback branch, including the sparse-checkout logic, the temp directory cleanup in finally, or the error wrapping.
Concrete cost: the fallback silently breaks (e.g. git not installed, sparse-checkout fails, temp dir leak on error) and no test catches it.
Suggested fix: add a test that mocks fetch to return a 500 on the contents API, then stubs execFile to verify the fallback produces the correct SkillPackageFile[]. A second test should verify a git failure produces github_skill_download_failed with status 502.
— qwen3.7-max via Qwen Code /review
| await onInstall({ name: name.trim(), scope, source: installSource }); | ||
| onOpenChange(false); | ||
| reset(); |
There was a problem hiding this comment.
[Suggestion] Double reset() call. onOpenChange(false) triggers the Dialog's onOpenChange handler which itself calls reset() when !nextOpen. So reset() is called twice on success. Same redundancy exists in the Cancel button.
Concrete cost: reset() is idempotent so no functional bug, but it signals unclear ownership of the cleanup path.
| await onInstall({ name: name.trim(), scope, source: installSource }); | |
| onOpenChange(false); | |
| reset(); | |
| await onInstall({ name: name.trim(), scope, source: installSource }); | |
| onOpenChange(false); |
Remove the explicit reset() call after onOpenChange(false), since the Dialog's onOpenChange handler already calls it.
— qwen3.7-max via Qwen Code /review
|
Review round triage for CI is green and the latest review reports no Critical/Major findings. This PR has already exceeded roughly five review rounds, so per repository guidance I am not expanding the diff for suggestion-only items. Already addressed on the current head: the 256-character compatibility boundary, both delete-route validations, source-root symlink rejection, bounded GitHub response streaming, rollback safety, ZIP actual-byte accounting, delete success feedback, and blocking dismissal during deletion. Deferred to follow-up work:
Not accepted in this PR:
No code commit was created for this review round. |
Review — feat(web-shell): add skill management pagesReviewed at head What the PR does
Verified end-to-end (not just from the diff)
Findings1. Leftover Staging and backup directories are created inside the skills base dir and contain a fully valid
Suggested fix inside this PR: sweep stale 2. GitHub URL path segments are never rejected for
One-line fix: reject any Hardening / UX
Nits
Test coverageStrong where it matters: the 569-line management suite covers rollback, commit-failure restore, cleanup-failure tolerance, zip bombs (declared and actual size), symlinked sources/entries, ref-injection ( VerdictSolid, security-conscious implementation — the two findings above are worth addressing before merge (#1 because the poisoned-discovery state is user-visible and un-recoverable via the API, #2 because it's a one-line fix to a token-bearing request path). Everything else is minor. 中文说明评审 — feat(web-shell): add skill management pages基于头提交 主要发现1. 残留的 2. GitHub URL 路径段未拒绝 加固 / UX
细节
结论实现扎实、安全意识强。建议合并前处理上述两个主要发现（#1 属用户可见且 API 无法自愈的状态污染，#2 是涉及 token 请求路径的一行修复），其余均为小问题。 |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| return skills.filter((skill) => { | ||
| if (level !== 'all' && skill.level !== level) return false; | ||
| if (status === 'disabled' && skill.status !== 'disabled') return false; | ||
| if (status === 'enabled' && skill.status === 'disabled') return false; |
There was a problem hiding this comment.
[Suggestion] Status filter treats error, warning, not_started, and unknown as enabled — the "enabled" filter only excludes status === 'disabled', so skills that failed to load appear in the "Enabled" filter bucket.
Failure scenario: a skill fails to load (status 'error'), user applies the "Enabled" status filter, and sees the errored skill listed there — potentially believing it is functional.
Suggested fix: either narrow the check to status === 'ok' (or 'ok' | 'warning'), or rename the filter label to 'not-disabled' to make the semantics explicit.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Keeping this unchanged: the filter represents the configured enabled/disabled state, not whether loading succeeded. An error/warning skill is still enabled unless its status is explicitly disabled, so classifying it as disabled would be misleading.
|
@wenshao 感谢这轮细致核对，两个主要问题都已在
新增回归覆盖：
本地验证： 其余建议中，workspace event 广播与本 PR 已确定的“进入列表时主动刷新”策略冲突，默认“已启用”筛选也是产品要求，因此未在本次修复中改变。 |
| @@ -0,0 +1,617 @@ | |||
| import { promises as fs } from 'node:fs'; | |||
There was a problem hiding this comment.
The test coverage for install paths is thorough — rollback, size limits, GitHub errors, symlink source rejection are all there. A few scenarios that exercise the validation in deleteWorkspaceSkill and filesFromFolder are missing:
deleteWorkspaceSkillwith askillDiroutsidebaseDiror with a mismatchedinstalledPath— the function has multi-layered path validation (lines 820-852) but no test verifies rejection.filesFromFolderwith a symlink in the middle of the tree (line 642-646) — only root-level symlink rejection is tested.- Encrypted ZIP entry rejection (line 525-528).
Not blocking, but these would round out the security-critical paths.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — its prompt was built, but no agent was launched with it that opened its brief, so the reverse-audit pass did not run.
Not reviewed: verification — its prompt was built, but no agent was launched with it that opened its brief, so the posted findings were not verified.
— qwen3.7-max via Qwen Code /review
| if (name.trim().length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { | ||
| res.status(400).json({ | ||
| error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, | ||
| code: 'invalid_skill_name', |
There was a problem hiding this comment.
[Suggestion] parseSkillInstallRequest validates the skill name for emptiness and length only, but does not apply the [A-Za-z0-9._-]+ regex that validateWorkspaceSkillName enforces. The delete route calls validateWorkspaceSkillName at the route layer; the install route defers that full validation to the service layer, after parseAndValidateClientId and buildWorkspaceCtx have already run.
Concrete cost: a request with { name: "my skill" } passes route validation, resolves the client ID, and builds a workspace context, only to fail at the service layer's validateWorkspaceSkillName. The install route does unnecessary work for a deterministically failing request.
| if (name.trim().length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { | |
| res.status(400).json({ | |
| error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, | |
| code: 'invalid_skill_name', | |
| const normalized = name.trim(); | |
| if ( | |
| !normalized || | |
| normalized === '.' || | |
| normalized === '..' || | |
| normalized.length > MAX_WORKSPACE_SKILL_NAME_LENGTH || | |
| !/^[A-Za-z0-9._-]+$/.test(normalized) | |
| ) { | |
| res.status(400).json({ | |
| error: 'Invalid skill name', | |
| code: 'invalid_skill_name', | |
| }); | |
| return undefined; | |
| } |
— qwen3.7-max via Qwen Code /review
| const [loadedSkillsReady, setLoadedSkillsReady] = useState(false); | ||
| const loadedSkillsRequestRef = useRef(0); | ||
| const reloadLoadedSkills = useCallback( |
There was a problem hiding this comment.
[Suggestion] reloadLoadedSkills is called only on initial connect/workspace-change and on new-session creation. After a skill toggle, install, or remove via SkillsManagerPage, the commands memo uses stale loadedSkills for slash-command autocomplete — SkillsManagerPage's internal reload() refreshes its own local hook state, which is a completely separate container with no shared cache or event bridge back to App.tsx.
Failure scenario: a user enables a skill via the Skills Manager panel. The skill's slash command does not appear in autocomplete until the next session is created or the WebSocket reconnects.
Suggested fix: pass an onSkillsChanged callback to SkillsManagerPage that calls reloadLoadedSkills(connection.workspaceCwd) after any toggle/install/remove, or introduce a workspace-level skills-changed event signal.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: chunk 14 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 6 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 4 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 7 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 3 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 10 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 2 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 13 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 11 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 8 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 9 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 5 — launched with a prompt that is not the one the CLI built. Not reviewed: chunk 12 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent was launched with it. Not reviewed: chunk 2 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 3 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 4 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 5 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 6 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 7 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 8 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 9 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 10 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 11 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 12 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 13 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 14 — its prompt was built, but no agent was launched with it. Not reviewed: Test coverage matrix (whole-diff) — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1b: Removed-behavior audit — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1c: Cross-file tracer — its prompt was built, but no agent was launched with it. Not reviewed: Agent 7: Build & test verification — its prompt was built, but no agent was launched with it. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it that opened its brief, so the reverse-audit pass did not run. Not reviewed: verification — its prompt was built, but no agent was launched with it that opened its brief, so the posted findings were not verified.
— qwen3.7-max via Qwen Code /review
| app.post( | ||
| '/workspaces/:workspace/skills/install', | ||
| deps.mutate({ strict: true }), |
There was a problem hiding this comment.
[Suggestion] Workspace-qualified install and delete routes (POST /workspaces/:workspace/skills/install and DELETE /workspaces/:workspace/skills/:name) have no tests at any level. The route test file workspace-skills.test.ts only covers the non-qualified variants. — Failure scenario: a regression in workspace-qualified path resolution (e.g. missing resolveWorkspaceRuntimeFromParam, wrong client-ID validation) would ship undetected, since workspace-qualified routes use different runtime resolution and client-ID validation than non-qualified ones.
| app.post( | |
| '/workspaces/:workspace/skills/install', | |
| deps.mutate({ strict: true }), | |
| // Add tests for both workspace-qualified routes in workspace-skills.test.ts | |
| // or workspace-qualified-rest.test.ts, mirroring the existing install/delete | |
| // tests but with a workspace registry mock. |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks. I am deferring this non-blocking coverage-only suggestion to a follow-up: it does not identify a current behavior defect, and this PR has already been through many review rounds. I am keeping the thread unresolved as requested for deferred items.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
6 participants
What this PR does
Adds a full Skill management experience to Web Shell.
/skillsopens an in-place page, and Skills also appear as the third Plugins tab. Users can search and filter Skills, inspect details, reference enabled Skills, enable or disable supported Skills, and see status and manual-reference guidance consistently with the MCP management UI.Adds trusted workspace APIs and SDK/WebUI bindings for installing and deleting workspace or global Skills. Installation supports a GitHub
SKILL.mdURL, a daemon-local folder path, or a bounded ZIP archive. The daemon validates package structure and metadata, confines filesystem mutations to managed Skill directories, performs staged replacement with rollback, and refreshes Skill state after successful mutations.Keeps slash-command completion synchronized with current Skill status when a new task starts, including both enabled-to-disabled and disabled-to-enabled transitions. Structured daemon errors are mapped to actionable Web Shell messages, including inactive parent extensions and common GitHub failures.
Why it's needed
Web Shell previously exposed Skill discovery through a transient chat response and had no management surface. Users could not inspect status, toggle supported Skills, or safely install and remove workspace/global Skills without editing files and settings manually. Cached command snapshots could also leave disabled Skills visible, or omit newly enabled Skills, when starting a new task.
Reviewer Test Plan
How to verify
/skills; confirm the in-place Skills page opens instead of adding a Skill list message to chat.SKILL.mdURL, an absolute daemon-local folder, and a ZIP containing either a rootSKILL.mdor one enclosing directory. Confirm success is reported on the list page.Evidence (Before & After)
Before:
/skillsrendered a transient list in chat, Skills had no Web Shell management page, and task creation could retain stale Skill slash commands.After:
/skillsopens the searchable management page with detail, status, toggle, upload, and delete actions; new tasks rebuild Skill commands from the latest workspace status.Tested on
Environment (optional)
Local Node.js workspace builds and focused Vitest suites. Browser E2E was not run because no authenticated daemon/Vite session was available.
Risk & Scope
workspace_skill_manage; existing read and toggle APIs remain compatible.Linked Issues
N/A
Pictures
中文说明
本 PR 做了什么
为 Web Shell 增加完整的技能管理体验。
/skills会打开页内管理页面，插件页面也将“技能”作为第三个标签。用户可以搜索和筛选技能、查看详情、引用已启用技能、启用或禁用受支持的技能，并以与 MCP 管理页面一致的方式查看状态和手动引用提示。增加受信任工作区的技能安装与删除 API，以及对应的 SDK/WebUI 接口。安装支持 GitHub
SKILL.md地址、daemon 本地文件夹路径和受大小限制的 ZIP 压缩包。daemon 会验证技能包结构和元数据，将文件系统修改限制在受管理的技能目录内，通过暂存目录和回滚安全替换文件，并在修改成功后刷新技能状态。新建任务时会根据最新技能状态同步斜杠命令，覆盖从启用到禁用和从禁用到启用两个方向。daemon 的结构化错误会映射为可操作的 Web Shell 文案，包括父扩展未启用和常见 GitHub 错误。
为什么需要
Web Shell 之前只能通过聊天中的临时消息展示技能列表，没有技能管理页面。用户无法查看状态、切换受支持的技能，也无法在不手动编辑文件和设置的情况下安全安装或删除工作区/全局技能。缓存的命令快照还可能导致新建任务时继续显示已禁用技能，或缺少刚启用的技能。
Reviewer 测试计划
如何验证
/skills，确认打开页内技能管理页面，而不是在聊天中追加技能列表消息。SKILL.md地址、daemon 本地绝对文件夹路径和 ZIP 安装工作区及全局技能。ZIP 可以直接包含根SKILL.md，也可以包含一个外层目录。确认列表页展示成功提示。证据（修改前后）
修改前：
/skills在聊天中展示临时列表，Web Shell 没有技能管理页面，新建任务可能保留过期的技能斜杠命令。修改后：
/skills打开支持搜索、详情、状态切换、上传和删除的管理页面；新建任务会根据最新工作区状态重建技能命令。测试平台
环境（可选）
本地 Node.js workspace 构建和针对性 Vitest 测试。由于没有可用的已认证 daemon/Vite 会话，未运行浏览器 E2E。
风险与范围
workspace_skill_manage检测修改能力，现有读取和状态切换 API 保持兼容。关联 Issue
N/A