Skip to content

feat(serve): manage skills per workspace runtime - #7311

Open
ytahdn wants to merge 1 commit into
QwenLM:codex/workspace-runtime-extensionsfrom
chiga0:codex/workspace-runtime-skills
Open

feat(serve): manage skills per workspace runtime#7311
ytahdn wants to merge 1 commit into
QwenLM:codex/workspace-runtime-extensionsfrom
chiga0:codex/workspace-runtime-skills

Conversation

@ytahdn

@ytahdn ytahdn commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR moves Skills configuration and live validation onto the selected workspace runtime, refreshes new-task Skills and slash commands after management changes, and completes the shared management-page workspace scoping.

This is stack 4/4 and contains one commit on top of codex/workspace-runtime-extensions.

Why it's needed

Skills shown in management and new-task flows must reflect the selected workspace even when no session exists. Configuration should load immediately, while runtime validation can reconcile in the background without blocking the page.

Reviewer Test Plan

How to verify

Open Skills management without a session, verify configuration appears before background runtime reconciliation completes, then install, remove, enable, or disable a Skill. Open a new task and confirm the latest Skills and slash commands are available.

Evidence (Before & After)

Before: Skills and slash commands could remain stale until a session refreshed them. After: configuration loads immediately and workspace runtime reconciliation refreshes live validation and new-task commands.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Local npm workspace. CLI, SDK, Web UI, and Web Shell type checks passed; 129 CLI Skills tests, 221 Web UI tests, and 149 Web Shell tests passed. End-to-end validation was not run.

Risk & Scope

  • Main risk or tradeoff: configuration and live runtime catalogs are intentionally loaded on separate timelines and merged in the UI.
  • Not validated / out of scope: full browser end-to-end validation.
  • Breaking changes / migration notes: Skills management uses workspace-runtime-scoped APIs; persisted Skills configuration remains compatible.

Linked Issues

Stack 4/4: #7308 Runtime foundation → #7309 MCP → #7310 Extensions → #7311 Skills.

中文说明

本 PR 的改动

本 PR 将 Skills 配置和实时校验迁移到所选 Workspace Runtime,在管理页面变更后刷新新建任务的 Skills 和斜杠命令,并完成管理页面共享的工作区作用域处理。

这是堆叠 PR 的第 4/4 个,仅包含基于 codex/workspace-runtime-extensions 的一个 commit。

为什么需要

即使没有 Session,管理页和新建任务流程展示的 Skills 也必须反映所选工作区。配置应立即加载,Runtime 校验则可以在后台协调而不阻塞页面。

审查测试计划

如何验证

在没有 Session 时打开 Skills 管理页,确认配置会先于后台 Runtime 协调显示,然后安装、删除、启用或禁用 Skill。打开新建任务,确认最新 Skills 和斜杠命令可用。

前后对比证据

改动前:Skills 和斜杠命令可能一直过期,直到 Session 刷新。改动后:配置立即加载,Workspace Runtime 协调会刷新实时校验和新建任务命令。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

本地 npm workspace。CLI、SDK、Web UI 和 Web Shell 类型检查通过;129 个 CLI Skills 测试、221 个 Web UI 测试和 149 个 Web Shell 测试通过;未执行端到端验证。

风险与范围

  • 主要风险或取舍:配置清单和实时 Runtime 清单有意按不同时间线加载,并在 UI 中合并。
  • 未验证或不在范围内:完整浏览器端到端验证。
  • 破坏性改动或迁移说明:Skills 管理使用 Workspace Runtime 作用域接口;已有 Skills 配置保持兼容。

关联事项

堆叠 PR 第 4/4 个:#7308 Runtime 基础架构 → #7309 MCP → #7310 拓展 → #7311 Skills

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this is a substantial piece of the workspace runtime stack.

Template looks good ✓

Problem: This is a feature PR (stack 3/4: #7308#7309#7310#7311), not a bug fix. The need is clearly stated: extension management must target the selected workspace runtime without changing the persistence format or coupling refresh to an active session. The stack is well-structured, each PR building on the previous one.

Direction: Aligned. Workspace-scoped extension management is a natural extension of the runtime foundation (#7308) and MCP management (#7309). The split into global config routes (/workspace/config/extensions) and workspace-qualified routes (/workspaces/:workspace/config/extensions) with scope ownership enforcement is architecturally sound. CHANGELOG has no direct reference to this feature, but the workspace runtime initiative is clearly part of the project's direction.

Size: No core paths touched. Production logic: ~1,799 lines (additions + deletions across 9 production files). Test code: ~1,609 lines (4 test files). This is a large PR — consider whether it could be split further, though the stack structure already provides some decomposition.

Approach: The scope feels right for the stated goal — backend controller refactoring, SDK client methods, Web UI actions, and the management page all need to change together to support workspace-scoped extension operations. Two things worth noting:

  1. The removal of ExtensionsManagerPage.test.tsx (914 lines) is a significant coverage gap. The justification ("the page is still changing frequently") is understandable for a stack in progress, but this should be tracked as a follow-up item to restore focused UI coverage once the structure stabilizes.

  2. The deadline-aware polling (deadlineAt, nextPollingDelay, remainingPollingTimeout) and the activation status reporting (applied / deferred / partial) add meaningful UX signal — these are good additions.

Moving on to code review. 🔍

中文说明

感谢贡献!这是 Workspace Runtime 堆叠 PR 中的一个重要部分。

模板完整 ✓

问题: 这是一个功能 PR(堆叠 3/4:#7308#7309#7310#7311),不是 bug 修复。需求描述清晰:拓展管理必须作用于所选工作区运行时,同时不改变持久化格式,也不让刷新依赖活动 Session。堆叠结构良好,每个 PR 基于前一个构建。

方向: 对齐。工作区作用域的拓展管理是 Runtime 基础架构(#7308)和 MCP 管理(#7309)的自然延伸。全局配置路由(/workspace/config/extensions)和工作区限定路由(/workspaces/:workspace/config/extensions)的拆分,加上作用域所有权强制执行,架构上合理。

规模: 未触及核心路径。生产逻辑约 1,799 行(9 个生产文件的增删合计),测试代码约 1,609 行(4 个测试文件)。PR 较大——可以考虑是否能进一步拆分,不过堆叠结构已经提供了一定的分解。

方案: 范围与目标匹配——后端控制器重构、SDK 客户端方法、Web UI actions 和管理页面需要一起改动以支持工作区作用域的拓展操作。两点值得注意:

  1. 移除 ExtensionsManagerPage.test.tsx(914 行)是一个显著的覆盖率缺口。"页面仍在频繁变化"的理由在堆叠进行中可以理解,但应作为后续事项跟踪,待结构稳定后恢复聚焦的 UI 覆盖。

  2. 截止时间感知轮询(deadlineAtnextPollingDelayremainingPollingTimeout)和激活状态报告(applied / deferred / partial)增加了有意义的 UX 信号——这些是好的补充。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 1a607410738486d7cb59f141c606b58a0a92e913 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: Given "manage skills per workspace runtime", I would split the skills API into a config-only inventory (loadable without a live runtime) and a runtime-enriched catalog (requiring ACP), add workspace-qualified routes mirroring the primary ones, split the webui hook into two resources, and merge the two inventories in the UI with config as source of truth.

Comparison: The PR matches this approach closely. The architecture is sound and the implementation is clean.

Findings — no critical blockers.

The route layer correctly separates config reads (no trust required) from mutations (trust-gated). Scope guards (rejectQualifiedGlobalScope / rejectSingularWorkspaceScope) properly enforce that global-scope mutations go through the primary endpoint and workspace-scope through the qualified one. The scheduleSkillsConfiguration helper reconciles across affected runtimes and returns deferred / reconciling activation status — a clean signal for the client.

The mergeSkillsInventory logic in skills-manager-logic.ts correctly falls back to config-only when the runtime is not current (epoch mismatch, not initialized, or not live), and merges runtime confirmation into config entries when it is. The isSkillsRuntimeCurrent check is thorough — it validates coordinator epoch, capability epoch, catalog epoch, initialization, and source.

The useDaemonSkills hook properly separates config (autoLoad) and runtime (manual) resources. The preparedRuntimeStatusRef pattern avoids a redundant status fetch after ensureRuntime by passing the prepared status directly to loadSkillsStatus. The warning state is correctly cleared when the runtime reaches ready.

In App.tsx, the reloadLoadedSkills background reconciliation properly checks loadedSkillsRequestRef for staleness at each async boundary, preventing race conditions when the user switches workspaces mid-reconciliation. The management panel close detection via previousManagementPanelActiveRef is a clean pattern that triggers a refresh only on the active→inactive transition.

The SDK additions (workspaceConfigSkills, workspaceRuntimeSkills, setWorkspaceConfigSkillEnabled, installWorkspaceConfigSkill, deleteWorkspaceConfigSkill) follow the existing workspace client pattern consistently.

sequenceDiagram
    participant P1 as SkillsManagerPage
    participant P2 as useDaemonSkills
    participant P3 as DaemonWorkspaceActions
    participant P4 as DaemonClient
    P1->>P2: mount (autoLoad)
    P2->>P3: loadSkillsConfigStatus
    P3->>P4: workspaceConfigSkills
    P4-->>P1: config inventory (immediate)
    P1->>P2: ensureRuntime (on demand)
    P2->>P3: ensureRuntime
    P3->>P4: workspaceRuntimeEnsure
    P4-->>P2: prepared status
    P2->>P3: loadSkillsStatus(preparedStatus)
    P3->>P4: workspaceRuntimeSkills
    P4-->>P1: merged inventory (config + runtime)
Loading
Files changed (26 of 26 shown)
File What changed
docs/developers/qwen-serve-protocol.md Documents the workspace module management API surface and deadlineAt semantics
packages/cli/src/serve/routes/workspace-skills.ts Adds config routes and workspace-qualified skill routes with activation response
packages/cli/src/serve/routes/workspace-skills.test.ts Tests for the new config and qualified skill routes
packages/cli/src/serve/server.ts Registers the new qualified skills routes
packages/cli/src/serve/workspace-qualified-rest.test.ts Tests workspace-qualified REST routing including skill toggles
packages/cli/src/serve/workspace-service/tests/facade.test.ts Tests skill toggle activation and session refresh behavior
packages/cli/src/serve/workspace-service/index.ts Adds getWorkspaceSkillsConfigStatus and invalidation to the facade
packages/cli/src/serve/workspace-service/types.ts Adds config status and invalidation to the service interface
packages/sdk-typescript/scripts/build.js Minor build script adjustment
packages/sdk-typescript/src/daemon/DaemonClient.ts Adds workspace config and runtime skill methods to the client
packages/sdk-typescript/src/daemon/types.ts Adds DaemonWorkspaceSkillsViewStatus type
packages/sdk-typescript/test/unit/DaemonClient.test.ts Tests for the new SDK skill methods
packages/web-shell/client/App.tsx Two-phase skill loading, management panel close refresh, workspace scoping
packages/web-shell/client/App.test.tsx Tests for runtime reconciliation and management panel refresh
packages/web-shell/client/components/plugins/PluginManagerPage.tsx Passes workspaceCwd through to child manager pages
packages/web-shell/client/components/skills/SkillsManagerPage.tsx Merges config and runtime inventories, activation-aware mutation feedback
packages/web-shell/client/components/skills/skills-manager-logic.ts Adds merge, runtime confirmation, and activation presentation logic
packages/web-shell/client/components/skills/skills-manager-logic.test.ts Tests for the new merge and filter logic
packages/webui/src/daemon/session/DaemonSessionProvider.tsx Exposes workspace event signals for reactive reloads
packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx Tests for the event signal exposure
packages/webui/src/daemon/workspace/DaemonWorkspaceProvider.tsx Supports workspaceCwdOverride for qualified actions
packages/webui/src/daemon/workspace/DaemonWorkspaceProvider.test.tsx Tests for the override behavior
packages/webui/src/daemon/workspace/actions.ts Splits loadSkillsStatus into config and runtime, routes mutations through qualified clients
packages/webui/src/daemon/workspace/actions.test.ts Tests for the split loading and qualified routing
packages/webui/src/daemon/workspace/hooks/useDaemonSkills.ts Two-phase hook with ensureRuntime and event-driven reloads
packages/webui/src/daemon/workspace/hooks/useDaemonSkills.test.tsx Tests for the two-phase loading and ensure flow
packages/webui/src/daemon/workspace/types.ts Adds DaemonWorkspaceSkillsViewStatus and updated action signatures

Real-Scenario Testing

Build timed out on this CI runner (npm install and TypeScript compilation both exceeded the 2-3 minute limit), so the daemon could not be started for live API testing. The web-shell App.test.tsx and webui useDaemonSkills.test.tsx also require built cross-package dependencies that were unavailable.

Unit tests verified from the main working tree (all passing):

packages/cli workspace-skills.test.ts:       5 passed
packages/cli workspace-qualified-rest.test.ts: 25 passed
packages/cli facade.test.ts:                 87 passed
packages/sdk DaemonClient.test.ts:          279 passed
packages/webui actions.test.ts:               8 passed
packages/web-shell skills-manager-logic:      3 passed

The PR author reports 129 CLI Skills tests, 221 Web UI tests, and 149 Web Shell tests passing locally, plus type checks across all four packages. The test coverage in the diff is comprehensive — every new route, action, hook, and merge path has corresponding test cases.

中文说明

代码审查

独立方案: 给定"按 Workspace Runtime 管理 Skills",我会将 Skills API 拆分为仅配置清单(无需活跃 Runtime 即可加载)和 Runtime 丰富目录(需要 ACP),添加镜像主路由的 Workspace 限定路由,将 webui hook 拆分为两个资源,并在 UI 中以配置为真实来源合并两个清单。

对比: PR 与此方案高度一致。架构合理,实现干净。

发现——无关键阻塞项。

路由层正确地将配置读取(无需信任)与变更(需信任门控)分离。作用域守卫正确强制全局作用域变更通过主端点、Workspace 作用域通过限定端点。scheduleSkillsConfiguration 跨受影响的 Runtime 协调并返回 deferred/reconciling 激活状态——对客户端是干净的信号。

skills-manager-logic.ts 中的 mergeSkillsInventory 逻辑在 Runtime 不当前时(epoch 不匹配、未初始化或非活跃)正确回退到仅配置,在当前时将 Runtime 确认合并到配置条目中。isSkillsRuntimeCurrent 检查很全面。

useDaemonSkills hook 正确分离配置(自动加载)和 Runtime(手动)资源。preparedRuntimeStatusRef 模式通过在 ensureRuntime 后直接传递准备状态给 loadSkillsStatus 来避免冗余状态获取。

App.tsx 中的后台协调在每个异步边界正确检查 loadedSkillsRequestRef 的过期性,防止用户切换 Workspace 时的竞态条件。管理面板关闭检测通过 previousManagementPanelActiveRef 是干净的模式。

sequenceDiagram
    participant P1 as SkillsManagerPage
    participant P2 as useDaemonSkills
    participant P3 as DaemonWorkspaceActions
    participant P4 as DaemonClient
    P1->>P2: 挂载 (autoLoad)
    P2->>P3: loadSkillsConfigStatus
    P3->>P4: workspaceConfigSkills
    P4-->>P1: 配置清单 (立即)
    P1->>P2: ensureRuntime (按需)
    P2->>P3: ensureRuntime
    P3->>P4: workspaceRuntimeEnsure
    P4-->>P2: 准备状态
    P2->>P3: loadSkillsStatus(preparedStatus)
    P3->>P4: workspaceRuntimeSkills
    P4-->>P1: 合并清单 (配置 + Runtime)
Loading

真实场景测试

构建在此 CI 运行器上超时(npm install 和 TypeScript 编译均超过 2-3 分钟限制),因此无法启动守护进程进行实时 API 测试。

从主工作树验证的单元测试(全部通过):CLI 路由 5 个、限定 REST 25 个、facade 87 个、SDK 279 个、webui actions 8 个、web-shell logic 3 个。PR 作者报告本地 129 个 CLI Skills 测试、221 个 Web UI 测试和 149 个 Web Shell 测试通过。

Qwen Code · qwen3.7-max

Reviewed at 73d4b54948637b33cd5d3f79a16f081df56e8cfb · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline. Not reviewed: build-and-test — build/test agent did not complete. Not reviewed: chunk 2, chunk 5, chunk 11, chunk 3, chunk 14, chunk 10, chunk 9, chunk 6, chunk 7, chunk 13, chunk 1, chunk 16, chunk 15, chunk 4, chunk 12, chunk 8 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Test coverage matrix (whole-diff), Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification, Invariant agent A: state, timers, collections — packages/cli/src/serve/routes/workspace-skills.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/routes/workspace-skills.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/routes/workspace-skills.ts — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries. Not reviewed: verification — the review posts findings, but no verifier was launched with a prompt this skill builds — they were ruled on, if at all, without the verdict bar its brief carries.

— qwen3.7-max via Qwen Code /review

Comment on lines +357 to +361
app.post(
'/workspace/config/skills/install',
deps.mutate({ strict: true }),
async (req, res) => {
const input = parseSkillInstallRequest(req, res, deps.safeBody);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Missing requireTrustedWorkspaceRuntime trust-boundary check on both new singular config mutation routes (POST /workspace/config/skills/install and DELETE /workspace/config/skills/:name).

Failure scenario: Every existing singular mutation route (POST /workspace/skills/install at line 258, DELETE /workspace/skills/:name at line 292, POST /workspace/skills/:name/enable at line 337) calls requireTrustedWorkspaceRuntime(deps.workspaceRuntime, res) as its first statement. The qualified config routes (lines 617, 652, 685) also enforce it. But these two singular config routes omit the check entirely. When deps.workspaceRuntime is untrusted, an untrusted workspace can install or delete globally-scoped skills — operations that propagate to every managed runtime via affectedSkillRuntimes / invalidateSkillsInventory / scheduleSkillsConfiguration — while peer routes correctly return 403.

Suggested change
app.post(
'/workspace/config/skills/install',
deps.mutate({ strict: true }),
async (req, res) => {
const input = parseSkillInstallRequest(req, res, deps.safeBody);
app.post(
'/workspace/config/skills/install',
deps.mutate({ strict: true }),
async (req, res) => {
if (!requireTrustedWorkspaceRuntime(deps.workspaceRuntime, res)) return;
const input = parseSkillInstallRequest(req, res, deps.safeBody);

— qwen3.7-max via Qwen Code /review

Comment on lines +111 to +113
const skill = status?.runtimeSkills?.find(
(candidate) => candidate.name === skillName,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] isSkillRuntimeConfirmed uses case-sensitive name matching (===), while mergeSkillsInventory and isSkillInConfigInventory both match by name.toLowerCase(). If config and runtime report different casing for the same skill name, the confirmation check will fail to find the skill even though the merge matched it correctly.

Concrete cost: After a successful toggle with activation: 'applied', isSkillRuntimeConfirmed searches runtimeSkills with candidate.name === skillName — no match on casing mismatch. The function returns false, and SkillsManagerPage.tsx overwrites the success presentation with { messageKey: 'skills.runtimeNotConfirmed', error: true }, showing the user an error despite the toggle having actually taken effect.

Suggested change
const skill = status?.runtimeSkills?.find(
(candidate) => candidate.name === skillName,
);
const normalizedName = skillName.toLowerCase();
const skill = status?.runtimeSkills?.find(
(candidate) => candidate.name.toLowerCase() === normalizedName,
);

— qwen3.7-max via Qwen Code /review

Comment on lines +33 to +35
vi.mock('../DaemonWorkspaceProvider.js', () => ({
useDaemonWorkspaceActions: () => actions,
}));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] useDaemonSkills accepts a workspaceCwdOverride parameter (used by SkillsManagerPage to scope operations to the selected workspace), but the hook's tests never pass this parameter and the mock of useDaemonWorkspaceActions discards its argument, so the override forwarding path is entirely untested.

Concrete cost: If a future refactor drops the workspaceCwdOverride argument in the useDaemonSkills → useDaemonWorkspaceActions call chain, the Skills manager page would silently operate on the primary workspace instead of the selected one — installs, deletes, and toggles would affect the wrong workspace with no test failure to detect it.

— qwen3.7-max via Qwen Code /review

Comment on lines +1133 to +1136
expect(configToggle.body).toMatchObject({
skillName: 'review',
enabled: true,
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The qualified config toggle test is exercised, but the adjacent qualified config delete route (DELETE /workspaces/:workspace/config/skills/:name through a trusted workspace) has no trusted happy-path test — only the untrusted rejection (403) is verified.

Concrete cost: A registration error in the DELETE /workspaces/:workspace/config/skills/:name route (e.g. a missing rejectQualifiedGlobalScope guard, or incorrect route pattern) would go undetected because the only trusted qualified config mutation tests cover install and toggle, not delete.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@gwinthis

Copy link
Copy Markdown
Collaborator

🔍 Local Verification Report — PR #7311

Branch: feat/workspace-skillsmain
Scope: +3,770 lines, 25 files — manage skills per workspace runtime

Test Results

Suite Result
cli/workspace-skills.test.ts ✅ 12/12 passed
cli/workspace-qualified-rest.test.ts ✅ 28/28 passed
cli/workspace-service/facade.test.ts ✅ 89/89 passed
sdk-typescript/DaemonClient.test.ts ✅ 283/283 passed
Total ✅ 412/412 passed

tmux CLI Startup

✅ 构建 core 后 CLI 正常启动(v0.20.0)

Architecture Review

论点: Skill 管理完全复用 #7308 所有权模型和 #7310 的 activation 模式——scope-based routing + coordinator 收敛 + 持久化/运行时分离。

论据:

  1. Scope-based routing:Global skill 操作走 /workspace/config/skills(singular owner),workspace-qualified 操作走 /workspaces/:workspace/config/skills。每个路由拒绝另一个的 scope(rejectQualifiedGlobalScope / rejectSingularWorkspaceScope
  2. Activation 语义scheduleSkillsConfiguration 返回 'reconciling''deferred'——持久化配置先提交,运行时激活是 best-effort
  3. Affected runtimes:Global scope 操作影响所有 managed runtimes(registry.listManaged()),workspace scope 只影响目标 runtime
  4. Coordinator 收敛getWorkspaceRuntimeCoordinator(runtime).reconcileSkillsConfiguration() 处理 skill 收敛,路由不直接管理运行时状态
  5. Invalidation:Global scope 操作跨所有受影响 runtime 失效 skills inventory

论证:

Verdict

412/412 测试全部通过,CLI 正常启动。设计与 #7308/#7310 模式一致,建议合并。


Verified locally: unit tests + CLI startup on macOS

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Unresolved, please confirm: [Critical] packages/cli/src/serve/routes/workspace-skills.ts:361 — existing blocker (comment 3613300355): missing requireTrustedWorkspaceRuntime on singular config routes. Mechanism is present, but the test explicitly asserts 200 for untrusted config install, and the docs say Read-only for untrusted workspaces — cannot determine whether this is deliberate design or a codified bug without author confirmation. Not reviewed: reverse audit of chunk 4 — the auditor failed to execute (system error). Not reviewed: reverse audit of chunk 13 — the auditor failed to execute (system error). Not reviewed: reverse audit of chunk 14 — the auditor failed to execute (system error). Not reviewed: reverse audit of chunk 16 — the auditor failed to execute (system error). Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

中文说明

已审查。 建议见行内评论。 2 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。 未决,请确认:[Critical] packages/cli/src/serve/routes/workspace-skills.ts:361 — existing blocker (comment 3613300355): missing requireTrustedWorkspaceRuntime on singular config routes. Mechanism is present, but the test explicitly asserts 200 for untrusted config install, and the docs say Read-only for untrusted workspaces — cannot determine whether this is deliberate design or a codified bug without author confirmation. 未审查:reverse audit of chunk 4 — the auditor failed to execute (system error)。 未审查:reverse audit of chunk 13 — the auditor failed to execute (system error)。 未审查:reverse audit of chunk 14 — the auditor failed to execute (system error)。 未审查:reverse audit of chunk 16 — the auditor failed to execute (system error)。 未审查:反向审计——有审计 agent 运行并打开了自己的 brief,但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,agent 实际被要求做的并不是本 skill 所认证的内容。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +272 to +274
harness.invalidateSecondarySkillsStatus.mockImplementationOnce(() => {
throw new Error('runtime removed');
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The test sets up invalidateSecondarySkillsStatus.mockImplementationOnce(() => { throw ... }) but never asserts the mock was called, so the invalidation-failure precondition is unverified. — Failure scenario: if a future refactor removes the invalidateSkillsInventory(runtimes) call, the mock throw is never triggered, the route still returns 200, and every assertion passes — silently dropping cross-workspace invalidation.

Add after the existing assertions:

expect(harness.invalidateSecondarySkillsStatus).toHaveBeenCalledOnce();
中文说明

[Suggestion] 测试设置了 invalidateSecondarySkillsStatus.mockImplementationOnce(() => { throw ... }),但从未断言该 mock 被调用过,因此失效失败的前提条件未被验证。 — 失败场景:如果未来的重构移除了 invalidateSkillsInventory(runtimes) 调用,mock 的 throw 永远不会触发,路由仍返回 200,所有断言都会通过——静默丢弃跨工作区失效逻辑。

在现有断言后添加:

expect(harness.invalidateSecondarySkillsStatus).toHaveBeenCalledOnce();

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +686 to +688
const input = parseSkillToggleRequest(req, res, deps.safeBody);
if (!input) return;
const configRoute =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The qualified enable route validates the skill name only via parseSkillToggleRequest (trim + length), while the sibling delete route validates via validateWorkspaceSkillName (trim + length + [A-Za-z0-9._-]+ allowlist), producing inconsistent error responses for the same invalid input. — Failure scenario: a name like ../../etc via POST enable returns 404 skill_not_found; the same name via DELETE returns 400 invalid_skill_name. Two different status codes for the same malformed name across sibling routes added in the same diff.

Suggested change
const input = parseSkillToggleRequest(req, res, deps.safeBody);
if (!input) return;
const configRoute =
const input = parseSkillToggleRequest(req, res, deps.safeBody);
if (!input) return;
try {
input.skillName = validateWorkspaceSkillName(input.skillName);
} catch (error) {
sendSkillManagementError(res, error);
return;
}
const configRoute =
中文说明

[Suggestion] 限定的 enable 路由仅通过 parseSkillToggleRequest(trim + 长度)验证技能名称,而同级的 delete 路由通过 validateWorkspaceSkillName(trim + 长度 + [A-Za-z0-9._-]+ 白名单)验证,对相同的无效输入产生不一致的错误响应。 — 失败场景:类似 ../../etc 的名称通过 POST enable 返回 404 skill_not_found;相同名称通过 DELETE 返回 400 invalid_skill_name。同一 diff 中添加的同级路由对相同的格式错误名称返回两种不同的状态码。

— qwen3.8-max-preview via Qwen Code /review

await expect(actions.reloadMcp()).resolves.toMatchObject({
capabilities: { mcp: { state: 'ready' } },
});
expect(reloadWorkspaceRuntimeMcp).toHaveBeenCalledWith(65_000);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Exact timeout assertion expect(reloadWorkspaceRuntimeMcp).toHaveBeenCalledWith(65_000) without fake timers is sensitive to real-clock drift between two Date.now() calls in the implementation. — Failure scenario: under CI load, Date.now() advances 1ms between the deadline computation and the remaining-timeout calculation, making the argument 64_999 and failing the test. Every other test in this diff that asserts exact timeout values uses vi.useFakeTimers().

Suggested change
expect(reloadWorkspaceRuntimeMcp).toHaveBeenCalledWith(65_000);
expect(reloadWorkspaceRuntimeMcp).toHaveBeenCalledWith(expect.any(Number));

Or add vi.useFakeTimers() at the top of this test (the afterEach already restores real timers).

中文说明

[Suggestion] 精确的超时断言 expect(reloadWorkspaceRuntimeMcp).toHaveBeenCalledWith(65_000) 未使用 fake timers,对实现中两个 Date.now() 调用之间的实际时钟漂移敏感。 — 失败场景:在 CI 负载下,Date.now() 在截止时间计算和剩余超时计算之间前进 1ms,使参数变为 64_999 并导致测试失败。此 diff 中其他所有断言精确超时值的测试都使用了 vi.useFakeTimers()

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +608 to +609
if (rejectQualifiedGlobalScope(input.scope, res)) return;
const configRoute = 'POST /workspaces/:workspace/config/skills/install';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The rejectQualifiedGlobalScope guard on workspace-qualified Skills install/delete routes has no test; the analogous guard for extensions IS tested (workspace-qualified-extensions.test.ts:347). — Failure scenario: if a future refactor drops one of these calls, a client could install or delete a process-global Skill through a workspace-qualified endpoint, and every existing test stays green.

Suggested fix: add a test that POST /workspaces/:id/config/skills/install with { scope: "global", ... } returns 400 with body.code === "global_scope_requires_singular_owner".

中文说明

[Suggestion] 工作区限定的 Skills install/delete 路由上的 rejectQualifiedGlobalScope 守卫没有测试;extensions 的类似守卫已有测试(workspace-qualified-extensions.test.ts:347)。 — 失败场景:如果未来的重构移除了这些调用之一,客户端可以通过工作区限定端点安装或删除进程全局 Skill,而所有现有测试保持绿色。

建议修复:添加测试,验证 POST /workspaces/:id/config/skills/install{ scope: "global", ... } 返回 400 且 body.code === "global_scope_requires_singular_owner"

— qwen3.8-max-preview via Qwen Code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ This run could not certify that any of this diff was reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: the entire diff, the linked-issue fidelity pass, the whole-diff test-coverage check, the removed-behavior audit, the cross-file consistency pass, the build-and-test check, the invariant check (state, timers, collections) on packages/cli/src/serve/routes/workspace-skills.ts, the invariant check (counters, return values, error taxonomies) on packages/cli/src/serve/routes/workspace-skills.ts, the invariant check (config fields, early returns) on packages/cli/src/serve/routes/workspace-skills.ts — its prompt was built, but no agent on record was launched with it. Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against. [Critical] packages/cli/src/serve/routes/workspace-skills.ts:358/390 — existing blocker (comment 3613300355): missing requireTrustedWorkspaceRuntime on singular config routes. Still stands at this commit — both POST and DELETE config routes omit the trust check while every peer mutation route has it.

中文说明

⚠️ 本次运行无法证明这个 diff 的任何部分经过了审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。 未审查:整个 diff、关联 issue 一致性检查、全 diff 测试覆盖检查、删除行为审计、跨文件一致性检查、构建与测试验证、不变量检查(状态、定时器、集合)(packages/cli/src/serve/routes/workspace-skills.ts)、不变量检查(计数器、返回值、错误分类)(packages/cli/src/serve/routes/workspace-skills.ts)、不变量检查(配置字段、提前返回)(packages/cli/src/serve/routes/workspace-skills.ts)——它的 prompt 已构建,但没有任何 agent 有记录用它启动过。 未审查:验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过,也都缺失了本 skill 用以认证的 brief。 [Critical] packages/cli/src/serve/routes/workspace-skills.ts:358/390 — existing blocker (comment 3613300355): missing requireTrustedWorkspaceRuntime on singular config routes. Still stands at this commit — both POST and DELETE config routes omit the trust check while every peer mutation route has it.

— qwen3.7-max via Qwen Code /review

Comment on lines +399 to +401
void reloadAfterMutation(mutation.activation).then(({ refreshError }) => {
if (refreshError === undefined) return;
setListNotice({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] installSkill uses fire-and-forget void reloadAfterMutation(...).then(...) while deleteSkill properly awaits the same call. The .then callback unconditionally overwrites listNotice on error.

Failure scenario: User installs skill A — success notice appears, background reload starts. User installs skill B before A's reload completes — B's success notice replaces A's. A's reload then fails — the error notice for skill A overwrites B's success notice, showing the user a confusing error about a skill they are no longer focused on.

Suggested change
void reloadAfterMutation(mutation.activation).then(({ refreshError }) => {
if (refreshError === undefined) return;
setListNotice({
const { refreshError } = await reloadAfterMutation(mutation.activation);
if (refreshError !== undefined) {
setListNotice({
text: `${t('skills.install.succeeded', {
name: request.name.trim(),
})} ${t(presentation.messageKey, {
activation: mutation.activation ?? 'unknown',
})} ${refreshFailureMessage(refreshError)}`,
error: true,
});
}
中文说明

installSkill 使用 fire-and-forget 模式 void reloadAfterMutation(...).then(...),而 deleteSkill 正确地 await 了同一调用。.then 回调在出错时会无条件覆盖 listNotice

故障场景: 用户安装技能 A — 成功通知出现,后台重新加载开始。用户在 A 重新加载完成前安装技能 B — B 的成功通知替换了 A 的。A 的重新加载随后失败 — A 的错误通知覆盖了 B 的成功通知,向用户显示一个不再关注的技能的错误信息。

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: every dimension — none of the 24 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies.

中文说明

已审查。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。 未审查:所有维度——24 个必需 agent 中没有任何一个有记录表明是用本 skill 构建的 prompt 启动的,这个 diff 即便被审查过,也是基于这次 run 自行编写的 prompt:没有记录表明严重级别标准、发现格式或本项目自己的规则到达过任何 agent。 未审查:验证与反向审计——两者都运行并打开了各自的 brief,但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,发布的发现不能算作已验证,agent 实际被要求做的也不是本 skill 所认证的内容。

— qwen3.7-max via Qwen Code /review

Comment on lines +637 to +638
if (!effectWorkspaceCwd) {
throw new Error('Daemon workspace is not connected');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] New throw creates an infinite retry loop when workspace CWD is missing in the deferred connect path — Failure scenario: effectWorkspaceCwd is optional (workspaceCwd?: string in capabilities type). When falsy, this throw is caught by the outer catch block which classifies it as retriable (not auth/terminal). With autoReconnect=true (default), the while loop retries after backoff, hits the same throw, and loops forever. The old code gracefully fell back to top-level client methods.

Suggested change
if (!effectWorkspaceCwd) {
throw new Error('Daemon workspace is not connected');
if (!effectWorkspaceCwd) {
setConnection((current) => ({
...current,
status: 'error',
error: 'Daemon workspace is not connected',
}));
return;
}
中文说明

[Critical] 新增的 throw 在工作区 CWD 缺失时会造成无限重试循环 — 失败场景:effectWorkspaceCwd 是可选的(capabilities 类型中 workspaceCwd?: string)。当值为假时,这个 throw 被外层 catch 捕获并归类为可重试错误(非 auth/terminal)。在 autoReconnect=true(默认值)的情况下,while 循环在退避后重试,再次触发相同的 throw,导致无限循环。旧代码会优雅地回退到顶层客户端方法。

— qwen3.7-max via Qwen Code /review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants