Skip to content

feat(daemon): enumerate installed extension skills locally - #11281

Merged
callmeYe merged 10 commits into
mainfrom
codex/daemon-extension-skill-catalog
Sep 15, 2026
Merged

callmeYe merged 10 commits into
mainfrom
codex/daemon-extension-skill-catalog

Conversation

@callmeYe

@callmeYe callmeYe commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Each loaded extension retains its own Skill defaults, combined with workspace overrides from the same store snapshot. Copied extensions with colliding IDs no longer cause owner lookup failures or lose disabled defaults in the local catalog.

Includes installed extension Skills in the daemon-local workspace catalog before an ACP child has published a snapshot. Active extensions participate in the existing project > user > extension > bundled precedence; inactive extensions appear as disabled management entries with their extension identity and metadata. Persistent extension Skill defaults and workspace overrides are combined with the existing settings opt-ins and disablements. Localized extension display names are projected from cached raw manifests on every provider response, using the existing workspace language resolution without rebuilding the directory cache.

Why it's needed

On a cold daemon without a session, the local fallback currently supplies no extensions, so installed extension Skills disappear from the workspace list. This delivers stage 2 of #11274 using the existing consistent extension store reader and manifest loaders, without creating a runtime Config, starting a child, initializing MCP, or executing hooks.

Reviewer Test Plan

How to verify

Start an isolated daemon with active and inactive extensions that each contain a Skill, without creating a session. The first workspace Skill response should include the active Skill and an inactive_extension entry for the disabled extension. Confirm extension display names, installed paths and invocability metadata are preserved. A project Skill with the same name should retain precedence over an active extension Skill, while the inactive extension entry retains its own identity.

Check an extension's default-disabled Skill, a persisted workspace override, a settings opt-in and a hard disable. Verify QWEN_CODE_SAFE_MODE=1, disabled extension discovery and untrusted workspaces do not load extension runtime context. Linked extensions and Agent Plugin manifests should use their existing loader behavior; an unreadable extension directory or failed store read should report an uninitialized error.

Evidence (Before & After)

Before: the isolated global CLI 0.18.5-preview.0 returned HTTP 200 with initialized:false and skills:[] before any session, despite on-disk fixtures. This older binary confirms the user-visible gap; focused source tests cover the current daemon-local provider.

After: five isolated local HTTP E2E scenarios passed: active/inactive catalog entries and precedence, a persisted workspace Skill override, workspace activation isolation, safe mode, and untrusted inventory. Daemon status reported zero sessions and ACP children; process inspection found no child and MCP/hook sentinels did not execute. Full evidence is posted in the separate test report comment.

Validation: full build, typecheck and bundle; 177 focused provider, mapping and facade tests covering the provider, status mapping, workspace facade and language resolution; changed-file ESLint and Prettier; two clean complete-diff self-audit passes and a local medium review with no findings.

Tested on

OS Status
macOS
Windows ⚠️ not tested locally
Linux ✅ independently verified by wenshao with real daemons (report)

Risk & Scope

An unusable extensions root or propagated store-read failure intentionally fails the entire pre-session catalog closed (initialized: false, empty skills), including bundled and project rows; per-artifact skip behavior remains owned by the shared loader. A contended extension-store consistency lock can hold a cold read through the shared retry policy (observed at about 27 seconds), and the first read can initialize extension-store state on disk when an extensions root exists. An absent extensions root needs no store creation. These are explicit stage-2 tradeoffs. Partial-success semantics, bounded/non-mutating store reads, provider coalescing/epoch changes and facade cache changes are excluded. Safe-mode coverage here refers to QWEN_CODE_SAFE_MODE=1; the pre-existing qwen serve --safe-mode propagation gap remains outside this PR and preserves parity with the child.

Strict scope boundary: Scope expansion is prohibited under all circumstances. This PR is limited to stage 2 of #11274: daemon-local enumeration of active/inactive extension Skills and the directly necessary tests and documentation. Do not opportunistically fix pre-existing issues outside this PR’s scope, including issues uncovered by CI or review. Record those separately for follow-up; do not add their fixes to this PR. Review revisions must remain within this boundary.

  • Main risk or tradeoff: cold reads now load installed extension metadata under the existing store consistency lock. Store initialization/legacy import and Agent Plugin data-directory creation retain their existing behavior.
  • Not validated / out of scope: making the daemon the sole list authority, expiry/global invalidation changes, configured-state fields, toggle/refresh policy and Web Shell session projections remain separate stages of tracking(daemon): Decouple Skill management from child and refresh explicit sessions in PRs under 1000 lines #11274. The existing provider cache is rebuilt at explicit invalidation points. The daemon's existing process-wide QWEN_HOME contract is unchanged.
  • Breaking changes / migration notes: no public response schema changes. Inactive extensions do not become active through Skill settings opt-ins.

Linked Issues

Refs #11274 (stage 2 only; does not close the tracking issue).

中文说明

本 PR 的改动

每个已加载 extension 使用自身的 Skill 默认值,并与同一 store 快照中的 workspace 覆盖合并。复制安装引发 ID 冲突时,本地目录不再因查错 owner 而失败,也不会丢失默认禁用状态。

让 daemon 在 ACP child 尚未提供快照时,就能从本地 workspace 目录列出已安装 extension 的 Skill。active extension 沿用 project > user > extension > bundled 的来源优先级;inactive extension 的 Skill 作为禁用的管理条目返回,并保留所属 extension 身份和元数据。持久化 extension Skill 默认值、workspace 覆盖与现有 settings 显式启用、禁用规则共同决定状态。本地化 extension 展示名在每次 provider 返回时从缓存的原始 manifest 投影,复用现有 workspace 语言解析,无需重建目录缓存。

为什么需要

冷启动且没有会话时,本地 fallback 目前提供空 extension 列表,导致已安装 extension 的 Skill 从 workspace 列表消失。本 PR 完成 #11274 的第 2 阶段,复用现有 extension store 一致性读取与 manifest loader,不构造运行时 Config、不启动 child、不初始化 MCP,也不执行 hooks。

Reviewer 验证计划

如何验证

在隔离 daemon 中准备分别包含 Skill 的 active 和 inactive extension,不创建会话。首次 workspace Skill 响应应包含 active Skill,以及 disabledReason 为 inactive_extension 的禁用条目。确认 extension 展示名、安装路径和可调用元数据保留。同名 project Skill 应继续优先于 active extension Skill,而 inactive extension 条目保留自己的身份。

检查 extension 默认禁用、持久化 workspace 覆盖、settings 显式启用及硬禁用。确认 QWEN_CODE_SAFE_MODE=1、禁用 extension 发现层级和未信任 workspace 不加载 extension 运行时上下文。linked extension 与 Agent Plugin manifest 应沿用既有加载行为;extension 目录不可读或 store 读取失败应明确返回未初始化错误。

前后证据

修改前:隔离的全局 CLI 0.18.5-preview.0 在未创建会话时返回 HTTP 200、initialized:false、skills:[],磁盘中已有测试资源。该旧版本用于确认可见缺口;定向源码测试覆盖当前 daemon-local provider。

修改后:五个隔离本地 HTTP E2E 场景通过,覆盖 active/inactive 目录及优先级、持久化 workspace Skill 覆盖、workspace 启停隔离、safe mode 和未信任目录。daemon 状态显示零会话和 ACP child,进程检查无子进程,MCP/hook 哨兵未执行。完整证据见独立测试报告评论。

验证:完整 build、typecheck、bundle;provider、状态映射、workspace facade 和语言解析共 177 项 provider、映射和 facade 定向测试;涉及文件 ESLint 和 Prettier;两轮完整 diff 自审无新增问题,本地 medium review 无发现。

测试平台

macOS 已验证;Windows 未在本地验证;Linux 已由 wenshao 使用真实 daemon 独立验证(报告)。

风险与范围

extensions 根目录不可用或 store 读取错误向上传播时,整个会话前目录会按明确契约 fail-closed(initialized: falseskills 为空),bundled 与 project 条目也不会部分返回;逐个制品的跳过行为仍由共享 loader 决定。extension-store 一致性锁被占用时,冷读取会沿用共享重试策略(实测约 27 秒),且存在 extensions 根目录时,首次读取可能在磁盘上初始化 extension-store 状态;根目录不存在时不创建 store。这些是第 2 阶段的明确取舍。本 PR 不引入部分成功、有界或无副作用的 store 读取、provider 并发合并或 epoch 改造,也不改动 facade 缓存。这里的 safe mode 验证特指 QWEN_CODE_SAFE_MODE=1;既有的 qwen serve --safe-mode 传播缺口不在本 PR 范围内,并继续与 child 保持一致。

严格范围约束:决不允许扩大本 PR 范围。本 PR 仅限 #11274 第 2 阶段:daemon 本地完整枚举 active/inactive extension Skill,以及直接必要的测试和文档。禁止顺手修复本 PR 范围外原先已存在的问题,包括 CI 或评审过程中发现的既有问题;这些问题只能单独记录并另行跟进,不得将修复加入本 PR。所有评审修订都必须遵守这一边界。

  • 主要成本:冷读取现在需要在现有 store 一致性锁内加载已安装 extension 元数据。store 初始化、旧格式导入和 Agent Plugin 数据目录创建沿用既有行为。
  • 范围之外:daemon 成为唯一列表来源、过期与全局失效调整、配置状态字段、开关和刷新策略、Web Shell 会话投影,仍按 tracking(daemon): Decouple Skill management from child and refresh explicit sessions in PRs under 1000 lines #11274 分阶段实施。现有 provider 缓存在显式失效点重建。daemon 现有进程级 QWEN_HOME 契约保持不变。
  • 兼容性:没有公共响应 schema 变化。Skill settings 显式启用不会启用 inactive extension。

关联 #11274,仅第 2 阶段,不关闭跟踪 issue。

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@callmeYe

callmeYe commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

E2E test report

Verified on macOS against the local 0.23.0 bundle for bb28cd1faaaeb81a73dac96c076ce723de0919c7.

Baseline: the globally installed 0.18.5-preview.0 daemon returned HTTP 200 with initialized:false, skills:[] before session creation, despite real on-disk fixtures. That older binary establishes the visible gap; the focused source tests target the current provider.

After: five isolated daemon runs passed. In every run, GET /workspace/skills was the first catalog request, before GET /workspace/config/skills. The trusted cold response was initialized and included active extension Skills plus inactive entries with inactive_extension.

  • Preserved extension identity, display name, installed path and standalone project/user Skills.
  • Preserved project precedence over an active same-name extension Skill and retained the inactive same-name extension entry.
  • Honored manifest Skill defaults, a persisted workspace Skill override and workspace-specific extension activation.
  • Excluded extension discovery in safe mode and in untrusted inventory; untrusted inventory ignored workspace Skill settings.
  • All runs asserted preheat.status=not_scheduled, zero active sessions, zero ACP children and channel.live=false. Process inspection found no child processes. MCP and SessionStart hook sentinels did not execute.

Controls: the harness set VITEST_WORKER_ID=catalog-e2e to disable startup preheat deterministically. This tests reads in the no-child state; it does not change or test production startup preheat. No sessions, model calls or browser UI were exercised. QWEN_HOME/runtime writes were isolated and all test-owned daemon processes were stopped. The standalone scanner also read existing user .agents/skills metadata without modifying it. A macOS /var versus /private/var fixture mismatch was corrected before the successful complete rerun.

Commands: npm run build && npm run typecheck && npm run bundle; package-local Vitest for the provider, mapping and facade (163/163 passing); changed-file ESLint and Prettier; node .qwen/scripts/daemon-extension-skill-verify.mjs. The ignored harness, report and raw JSON remain in this task's worktree under .qwen/e2e-tests/ and .qwen/scripts/.

PR scope is 382 additions + 30 deletions = 412 lines, including tests and design documentation.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — and for writing the design doc first, which is exactly what the direction review on #11274 asked for.

Template looks good ✓

Problem: real, and confirmed by reading main rather than taken on faith. workspace-skills-status.ts shimmed getActiveExtensions: () => [], and SkillManager sources extension-level skills only from that getter (packages/core/src/skills/skill-manager.ts:1010-1014), so the daemon-local fallback could only ever yield project/user/bundled. The base file documents that omission as intentional, so this isn't fixing a mistake — it's building the extension context the old comment claimed can't exist outside a child. That's a genuine gap on the pre-first-prompt path, not theoretical hardening.

Direction: aligned, and importantly narrower than the tracking issue proposed. #11274 wanted the daemon-local provider to become the sole list authority and drop child preference; this PR keeps "the live child, when present, stays authoritative" intact and only fills the extension hole in the fallback. That matters, because the direction review on #11274 called the invariant flip a maintainer-level call — this PR sidesteps it. Worth saying plainly though: #11274 is your own issue, still need-discussion and P3, with no maintainer sign-off in the thread, so the plan this implements is self-authored. Two of the review points below land on that boundary.

Size: Stage 0 doesn't apply — no core paths (packages/core/src/**, packages/*/src/{auth,providers,models,config,tools,services}/**) and no cross-package change. For reference: 94 production lines in one file, 290 test lines, 28 lines of design doc. Well under every threshold, and inside the ≤1000-line budget #11274 set for itself.

Approach: scope feels right and the shape deliberately mirrors the ACP child's producer in acpAgent.ts — same inactive-extension append, same { disabled: true } mapping, same final localeCompare sort. That parity is the correct instinct, since runtime/workspace-skills-mapping.ts exists precisely so the two listings can't drift. One reuse gap and two design questions go in the code review; the one I'd flag here is that the daemon already has a canonical unbound-manager factory (createExtensionManager in workspace-extensions-controller.ts) which supplies a per-workspace locale, and this constructs ExtensionManager directly without one.

No drive-by refactors or unrelated churn — every hunk serves the stated goal. The two reordered test expectations are mock artifacts, not a behavior change; I checked why in the review.

Risk: no elevated risk signals — none of the three files match the revert-correlated path set. The new surface is a daemon public contract (/workspace/skills fallback and /workspace/config/skills), so the review treats response content, scope ownership and failure semantics as the thing to verify.

Moving on to code review. 🔍

中文说明

感谢贡献 —— 也感谢先写了 design doc,这正是 #11274 方向性 review 所要求的。

模板完整 ✓

问题: 真实存在,而且是通过阅读 main 确认的,不是照单全收。workspace-skills-status.ts 里 shim 的 getActiveExtensions: () => [],而 SkillManager 的 extension 级 Skill 来自这个 getter(packages/core/src/skills/skill-manager.ts:1010-1014),所以 daemon 本地 fallback 永远只能给出 project/user/bundled。基线代码把这个省略记录为有意为之,因此这不是在修一个失误 —— 而是在补上旧注释声称"child 之外不可能存在"的 extension 上下文。这是首次 prompt 之前那条路径上的真实缺口,不是理论性加固。

方向: 对齐,而且比 tracking issue 的方案更收窄,这点很重要。#11274 想让 daemon 本地 provider 成为列表的唯一来源并移除 child 优先;本 PR 保留了"live child 存在时仍是权威",只填补 fallback 里的 extension 空缺。这一点关键,因为 #11274 的方向性 review 把"翻转该不变量"称为维护者级别的决定 —— 本 PR 绕开了它。不过也要直说:#11274 是您自己提的 issue,目前仍是 need-discussion + P3,线程里没有维护者签字,所以本 PR 实现的计划是自拟的。下面两条 review 意见正好落在这个边界上。

规模: Stage 0 不适用 —— 没有触及核心路径(packages/core/src/**packages/*/src/{auth,providers,models,config,tools,services}/**),也不是跨包改动。供参考:单个文件 94 行生产代码、290 行测试、28 行 design doc。远低于所有阈值,也在 #11274 自设的 ≤1000 行预算内。

方案: 范围合理,且形态明确对齐 acpAgent.ts 中 ACP child 的 producer —— 相同的 inactive extension 追加逻辑、相同的 { disabled: true } 映射、相同的末尾 localeCompare 排序。这种对齐是正确的直觉,因为 runtime/workspace-skills-mapping.ts 存在的意义就是让两份列表不会漂移。一个复用缺口和两个设计问题放在代码审查里;这里先点出一条:daemon 已有规范的 unbound manager 工厂(workspace-extensions-controller.tscreateExtensionManager),它会提供 per-workspace locale,而本 PR 直接构造 ExtensionManager 且没有传 locale。

没有顺手重构或无关改动 —— 每个 hunk 都服务于既定目标。两处测试期望顺序的调整是 mock 造成的假象,不是行为变化;审查里我核对了原因。

风险: 无升级风险信号 —— 三个文件都不匹配与 revert 相关的路径集合。新增面是 daemon 的公开契约(/workspace/skills fallback 与 /workspace/config/skills),因此审查把响应内容、scope 归属和失败语义作为需要验证的重点。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 9585979, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

health-deep-with-session

field PR base (before) this PR (after)
activeWorkStaleMs 8 6

Qwen Code · serve A/B

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

I read the diff against main and then read the surrounding code — the ACP child's producer, Config.isSkillEnabled, SkillManager, ExtensionManager.refreshCache and ExtensionStore. Static reading only; nothing was built or executed (unattended CI run, and PR code is never run here).

The shape is right. Mirroring acpAgent.ts's producer rather than inventing a second mapping is the correct instinct, and the gating is genuinely careful: workspaceTrusted && !safeMode && !disabledLevels.has('extension') means the untrusted, inert-untrusted and safe-mode providers never construct an ExtensionManager at all, and nothing extension-derived executes — refreshTools is untouched, MCP configs only pass through filterMcpConfig, hooks get parsed into extension.hooks and never run. Scope ownership holds too: the manager is keyed to the requested workspaceCwd and never reaches for the primary runtime.

Three things I'd want a human to look at.

1. locale is never passed, so extension display names come back in English. new ExtensionManager({ workspaceDir, isWorkspaceTrusted }) omits locale, which defaults to 'en' (extensionManager.ts:521), and that locale is what resolves displayName and description through resolveExtensionConfigLocale (extensionManager.ts:1854, extension/i18n.ts:62-97). Every other producer of this field passes one: the child via Config (config.ts:2943, locale: params.locale) and the daemon's own extension routes via createExtensionManagerresolveExtensionLocale(workspaceDir, workspaceTrusted) (workspace-extensions-controller.ts:317), which reads general.language from that workspace's settings. So for a user on general.language: "zh" with an extension shipping a locale map, /workspace/config/skills and the pre-child /workspace/skills fallback return the English extensionDisplayName, and the same field flips to Chinese the moment a child answers. That's precisely the drift runtime/workspace-skills-mapping.ts exists to prevent — in content rather than in shape.

The tests can't catch it: writeExtension emits displayName: "<name> display" as a plain string, and resolveLocalizableString returns plain strings untouched regardless of locale.

Fix is small, and it's a reuse point rather than new code — settings is already loaded 40 lines above, so settings.merged.general?.language is in hand. resolveExtensionLocale is a module-local const in workspace-extensions-controller.ts; exporting and reusing it beats re-deriving the auto / detectSystemLanguage / resolveSupportedLanguage chain a second time.

2. refreshCache() is not a read, and this route was previously read-only. The PR body says the change avoids "creating a runtime Config, starting a child, initializing MCP, or executing hooks" — all true, but it's read-only with respect to extension code, not with respect to the filesystem. Traced: refreshCacherefreshCacheWithSnapshotextensionStore.readConsistentwithLock, which calls prepareDirectories() (mkdir + chmod 0o700 on extensionsDir, storeDir, and staging/rollback/transactions), then recoverCorruptStateUnlocked() and recoverTransactionsUnlocked(), then ensureInitializedUnlocked() — which bumps generation and calls writeSnapshotUnlocked() whenever anything changed, and on a first-ever cold read there is no snapshot at all, so it mints a default policy per loaded extension and writes state.json. Agent Plugin manifests additionally create their data dir (loadAgentPluginMcpServers(..., { createDataDir: true }), extensionManager.ts:1660-1664).

The latency half matters more than the writes. That lock is proper-lockfile with retries: 60, factor 1.2, 50–500 ms randomized — roughly 25 s of retrying before it throws ExtensionStoreBusyError, which the provider's outer catch turns into initialized: false, skills: [], emptying project/user/bundled skills as well. And this isn't only the pre-child fallback: GET /workspace/config/skills and GET /workspaces/:workspace/config/skills call the provider directly (server.ts:953-964routes/workspace-skills.ts:379,633), and every skills toggle invalidates first then re-reads (routes/workspace-skills.ts:293-294,614-618). So the file's own promise — "the daemon can answer /workspace/skills instantly" — now has a lock-contention path behind it, worst case a ~25 s hang coalesced across all concurrent readers by inFlightWorkspaceSkillsStatus, then an empty list.

I'm not calling this a defect: fail-closed is consistent with how the same function already treats an unreadable project/user skills dir, the child does the identical store read at session start, and the PR body does flag the lock. But the body frames it as a consistency guarantee, not as a latency and write-on-read change to a route that had neither. Worth a conscious decision — e.g. should extension-store contention degrade to "extension level unavailable, rest of the catalog intact" rather than blanking everything?

3. Nothing invalidates this cache when an extension changes. Extension mutations call runtime.workspaceService.invalidateWorkspaceSkillsStatus() (routes/workspace-extensions.ts:781,791), which invalidates the per-runtime provider. The process-global trustedSkillsConfigStatus / untrustedSkillsConfigStatus pair that getSkillsConfigStatus reads is only invalidated by invalidateSkillsConfigStatus, and that symbol appears nowhere in the extension routes — only server.ts (definition, onWorkspaceRemoved) and routes/workspace-skills.ts. So after an extension install / update / enable / disable, /workspace/config/skills keeps serving the pre-mutation ExtensionManager snapshot, cached per workspaceCwd for the life of the daemon process, until a skills-route mutation or a workspace removal happens to clear it.

Before this PR that gap was invisible, because the route showed no extension skills at all. After it, the route can show a present-but-stale extension list, which is a worse failure mode than an absent one. #11274 does list immediate invalidation after extension commits as in-scope for the program, and this PR explicitly defers invalidation policy to a later stage — so this may be entirely intentional. Flagging it so the deferral is a decision rather than an oversight, and so whichever stage owns the hook knows this consumer needs it too.

One drift risk, non-blocking. The inline enabled expression re-derives the extension branch of Config.isSkillEnabled (config.ts:6211-6238) and matches the owning extension by name alone, where Config also matches filePath membership. That stricter match is incidentally what shields Config from getExtensionSkillState's "does not belong to extension" throw (extensionManager.ts:782-790). It's safe here — SkillManager sources extension-level skills directly from extension.skills (skill-manager.ts:1010-1014), so the name is always present — but the invariant lives in a different package, and if it ever broke the throw would land in the outer catch and blank the whole listing. A short comment, or a shared helper both call sites use, would pin it.

Checked and fine, so a reviewer doesn't have to re-do it. The added .sort((a, b) => a.name.localeCompare(b.name)) is parity with the child (acpAgent.ts:7256-7258), not a reordering of existing output — listSkills() already returns name-sorted results (skill-manager.ts:334), so the sort only places the newly appended inactive entries. The two flipped test expectations (enabled/disableddisabled/enabled) are artifacts of those tests mocking listSkills with an unsorted literal; no response-ordering regression. The fs.readdir(Storage.getUserExtensionsDir()) probe is load-bearing, not redundant: loadExtensionsFromExtensionsDir swallows readdir failures with catch { return [] } (extensionManager.ts:1607-1611), so without the probe an unreadable extensions dir would report initialized: true over an empty catalog. It also probes exactly the directory refreshCache will read, since no custom store is injected. Inactive entries carry installedPath and invocability metadata because extension.skills comes from the same loadSkillsFromDir / loadAgentPluginSkills loaders.

sequenceDiagram
    participant P1 as HTTP route
    participant P2 as Workspace facade
    participant P3 as Local provider
    participant P4 as ExtensionManager
    participant P5 as ExtensionStore
    participant P6 as SkillManager
    P1->>P2: GET workspace skills
    P2->>P3: child answer and cached answer both unavailable
    P3->>P3: loadSettings, derive safeMode and disabledLevels
    alt trusted, not safe mode, extension level enabled
        P3->>P3: readdir probe on the user extensions dir
        P3->>P4: construct unbound manager for this workspace
        P4->>P5: refreshCache under the store lock
        Note over P5: mkdir plus chmod, recovery passes,<br/>state.json write when changed
        P5-->>P4: snapshot plus persisted activation
    else untrusted, safe mode, or level disabled
        P3->>P3: no manager constructed, no store touched
    end
    P3->>P6: listSkills via the shim getActiveExtensions
    P6-->>P3: name-sorted project, user, extension, bundled
    P3->>P3: resolve enabled state, append inactive entries, sort
    P3-->>P2: initialized status
Loading

Testing

What this section carries: the PR's own CI check results, read through the API. This run built nothing, ran no test, and started no daemon — under the skill's rules PR-derived code is never executed in CI, and the agent env holds a write PAT that executed code could read.

Zero failures at review time. One PR-CI workflow run (Qwen Code CI) is still in progress on web-shell E2E Smoke; everything else on this commit has settled. verify and tmux-testing are skipped because no maintainer has triggered them — not because they passed.

The unit suite ran on Linux only. Test (macos-latest) and Test (windows-latest) are skipped by the workflow's matrix rules, not red — so the eight new test cases in workspace-skills-status.test.ts have never executed on macOS or Windows in CI. Given the new code is filesystem-shaped (readdir probes, extension dir scans, mkdir/chmod in the store path) and the author reports testing locally on macOS only, Windows is currently covered by nobody.

Check Conclusion
Test (ubuntu-latest, Node 22.x) success
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped
Lint & Static (ubuntu-latest, Node 22.x) success
Serve A/B (ubuntu-latest, Node 22.x) success
Integration Tests (no-AK, No Sandbox) success
Integration Tests (CLI, No Sandbox) skipped
Real daemon E2E / Java 11 success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) in_progress
TUI parity snapshots (ink vs opentui) success
OpenTUI no-flicker gate success
Desktop Shell (ubuntu-22.04) success
Desktop Shell (windows-2022) success
verify skipped (not triggered)
tmux-testing skipped (not triggered)
triage / review-pr in_progress (bot orchestration, not PR CI)

Not verified, and why:

  • not verified: any live behaviour. No daemon was started and no HTTP response was observed in this run. Every behavioural statement above is derived from reading source.
  • not verified: that the cold response actually changes on the PR's own base build. The "Before" evidence is against a globally installed 0.18.5-preview.0 binary — a different, much older build — so it establishes that the gap existed at some point, not that it exists at this PR's merge base. The A/B against main is the missing half.
  • not verified: "no child process, no MCP, no hook execution." I confirmed by reading that this code path cannot execute extension code, which is strong. The author's process inspection and sentinel checks are the author's claim from a macOS harness, not independently re-run here.
  • not verified: non-English locale behaviour. Finding 1 above; no test exercises a locale-mapped displayName.

The author's E2E report in this thread (five isolated daemon scenarios, 163 focused tests, preheat.status=not_scheduled, zero sessions and zero ACP children) is the author's claim, attributed as such — it is not evidence this review reproduced, and it is not counted toward the testing verdict above.

Sandboxed verification would settle this: @qwen-code /verify — that the cold daemon-local response on this base build goes from extension-free to including active plus inactive_extension entries is not observable from the diff, and the "Before" half of the claim currently rests on an unrelated 0.18.5-preview.0 binary rather than an A/B against main. A load-bearing run would also settle that no child is spawned and no MCP or hook sentinel fires, which is the safety property the whole design rests on and which static reading can support but not prove. The author has write access, so /verify needs no sponsor; @qwen-code /tmux is the lane if a maintainer would rather see the Web Shell's pre-first-prompt skill autocomplete directly.

中文说明

代码审查

我把 diff 与 main 对照阅读,然后读了周边代码 —— ACP child 的 producer、Config.isSkillEnabledSkillManagerExtensionManager.refreshCacheExtensionStore。仅为静态阅读;本次是无人值守 CI,没有构建或执行任何东西(此流程也从不运行 PR 代码)。

整体形态是对的。对齐 acpAgent.ts 的 producer 而不是另造一套映射,是正确的直觉;信任门控也确实细致:workspaceTrusted && !safeMode && !disabledLevels.has('extension') 意味着 untrusted、inert-untrusted 和 safe mode 三种 provider 根本不会构造 ExtensionManager,也不会执行任何 extension 派生内容 —— refreshTools 未被触及,MCP 配置只经过 filterMcpConfig,hooks 只被解析进 extension.hooks 而从不运行。scope 归属也成立:manager 以请求的 workspaceCwd 为键,绝不会回落到 primary runtime。

有三点希望由人来看。

1. 从未传入 locale,导致 extension 展示名以英文返回。 new ExtensionManager({ workspaceDir, isWorkspaceTrusted }) 省略了 locale,默认为 'en'extensionManager.ts:521),而这个 locale 正是通过 resolveExtensionConfigLocale 解析 displayNamedescription 的依据(extensionManager.ts:1854extension/i18n.ts:62-97)。该字段的其他所有 producer 都传了 locale:child 经由 Configconfig.ts:2943locale: params.locale),daemon 自己的 extension 路由经由 createExtensionManagerresolveExtensionLocale(workspaceDir, workspaceTrusted)workspace-extensions-controller.ts:317),后者从该 workspace 的设置里读 general.language。因此对于 general.language: "zh" 且 extension 提供了 locale map 的用户,/workspace/config/skills 和首次 prompt 之前的 /workspace/skills fallback 会返回英文 extensionDisplayName,而同一字段在 child 应答的那一刻变成中文。这正是 runtime/workspace-skills-mapping.ts 存在的目的所要防止的漂移 —— 只不过发生在内容层面而非结构层面。

测试抓不到它:writeExtension 写出的 displayName: "<name> display" 是普通字符串,而 resolveLocalizableString 对普通字符串原样返回,与 locale 无关。

修复很小,而且属于复用而非新写代码 —— 上方 40 行处已经加载了 settingssettings.merged.general?.language 就在手边。resolveExtensionLocale 目前是 workspace-extensions-controller.ts 里的模块内 const;把它导出复用,好过第二次重新推导 auto / detectSystemLanguage / resolveSupportedLanguage 这条链。

2. refreshCache() 不是纯读,而这条路由此前是纯读的。 PR 描述说改动避免了"构造运行时 Config、启动 child、初始化 MCP、执行 hooks" —— 都成立,但这是相对于 extension 代码只读,不是相对于文件系统只读。追踪链路:refreshCacherefreshCacheWithSnapshotextensionStore.readConsistentwithLock,其中调用 prepareDirectories()(对 extensionsDirstoreDir 以及 staging/rollback/transactions 执行 mkdir + chmod 0o700),随后是 recoverCorruptStateUnlocked()recoverTransactionsUnlocked(),再是 ensureInitializedUnlocked() —— 只要有变更就会提升 generation 并调用 writeSnapshotUnlocked();而首次冷读时根本不存在 snapshot,于是它会为每个已加载 extension 生成默认 policy 并写出 state.json。Agent Plugin manifest 还会额外创建自己的 data 目录(loadAgentPluginMcpServers(..., { createDataDir: true })extensionManager.ts:1660-1664)。

比写入更要紧的是延迟。该锁是 proper-lockfileretries: 60、factor 1.2、50–500 ms 随机化 —— 抛出 ExtensionStoreBusyError 之前大约会重试 25 秒,而这个异常会被 provider 外层 catch 转成 initialized: false, skills: [],连 project/user/bundled 也一并清空。并且这不只是 child 之前的 fallback:GET /workspace/config/skillsGET /workspaces/:workspace/config/skills直接调用该 provider 的(server.ts:953-964routes/workspace-skills.ts:379,633),而每次 Skill 开关都会先失效再重读(routes/workspace-skills.ts:293-294,614-618)。所以文件自身那句承诺 —— "daemon 可以即时应答 /workspace/skills" —— 现在背后多了一条锁竞争路径,最坏情况是约 25 秒的挂起(并被 inFlightWorkspaceSkillsStatus 合并到所有并发读者身上),然后返回空列表。

我不把这判为缺陷:fail-closed 与同一函数对不可读 project/user skills 目录的既有处理一致,child 在会话启动时做的是完全相同的 store 读取,PR 描述也确实提到了这把锁。但描述把它写成一致性保证,而没有写成对一条此前既无锁也无写入的路由引入的延迟与"读路径写盘"变化。值得做一次明确决定 —— 例如:extension store 竞争时,是否应降级为"extension 层不可用、目录其余部分完好",而不是清空全部?

3. extension 变化时没有任何机制失效这份缓存。 extension 变更调用的是 runtime.workspaceService.invalidateWorkspaceSkillsStatus()routes/workspace-extensions.ts:781,791),失效的是per-runtime provider。而 getSkillsConfigStatus 读取的那对进程级单例 trustedSkillsConfigStatus / untrustedSkillsConfigStatus,只能由 invalidateSkillsConfigStatus 失效,而这个符号在 extension 路由里完全没有出现 —— 只在 server.ts(定义、onWorkspaceRemoved)和 routes/workspace-skills.ts 中。因此 extension 安装/更新/启停之后,/workspace/config/skills 会继续返回变更前的 ExtensionManager 快照,按 workspaceCwd 缓存在 daemon 进程的整个生命周期里,直到某次 Skill 路由变更或 workspace 移除恰好把它清掉。

在本 PR 之前这个缺口是不可见的,因为该路由根本不显示 extension Skill。本 PR 之后,该路由可能显示一份"存在但陈旧"的 extension 列表 —— 这比"缺失"是更糟的失败模式。#11274 确实把"extension 提交后立即失效受影响目录"列入整个计划的范围,而本 PR 明确把失效策略推迟到后续阶段 —— 所以这完全可能是有意的。提出来是为了让这个推迟成为一个决定而非疏漏,也让负责该钩子的那个阶段知道这个消费者同样需要它。

一个漂移风险,非阻塞。 内联的 enabled 表达式重新推导了 Config.isSkillEnabled 的 extension 分支(config.ts:6211-6238),且仅按 name 匹配所属 extension,而 Config 还匹配 filePath 归属。那个更严格的匹配顺带正是 Config 免于 getExtensionSkillState 抛出"不属于该 extension"异常的保护(extensionManager.ts:782-790)。这里是安全的 —— SkillManager 的 extension 级 Skill 直接来自 extension.skillsskill-manager.ts:1010-1014),所以名字必然存在 —— 但这个不变量位于另一个包里,一旦它被打破,异常就会落进外层 catch 并清空整份列表。加一句注释,或让两处调用同一个共享 helper,就能把它钉住。

已核对且无问题的部分,省得 reviewer 重做一遍。 新增的 .sort((a, b) => a.name.localeCompare(b.name)) 是与 child 对齐(acpAgent.ts:7256-7258),并不是对既有输出重排序 —— listSkills() 本身就返回按名称排序的结果(skill-manager.ts:334),所以这个排序只影响新追加的 inactive 条目的落位。两处测试期望顺序的翻转(enabled/disableddisabled/enabled)是那两个测试用无序字面量 mock listSkills 造成的假象;不存在响应顺序回归。fs.readdir(Storage.getUserExtensionsDir()) 这个探测是承重的,不是冗余:loadExtensionsFromExtensionsDircatch { return [] } 吞掉了 readdir 失败(extensionManager.ts:1607-1611),因此没有这个探测,不可读的 extensions 目录会在空目录之上报告 initialized: true。它探测的也正好是 refreshCache 将要读取的目录,因为没有注入自定义 store。inactive 条目带有 installedPath 与可调用元数据,是因为 extension.skills 来自同一套 loadSkillsFromDir / loadAgentPluginSkills 加载器。

(时序图见上,中文不再重复。)

测试

本节承载的证据: 通过 API 读取的、该 PR 自身 CI 检查结果。本次运行没有构建、没有跑测试、没有启动 daemon —— 按 skill 规则,CI 中从不执行 PR 派生代码,且 agent 环境持有可被执行代码读取的 write PAT。

审查时点零失败。该 commit 上仍有一个 PR CI workflow run(Qwen Code CI)在 web-shell E2E Smoke 上运行中,其余均已结束。verifytmux-testing 为 skipped,是因为没有维护者触发,不是因为通过。

单元测试仅在 Linux 上运行Test (macos-latest)Test (windows-latest) 是被 workflow 矩阵规则 skipped,不是失败 —— 也就是说 workspace-skills-status.test.ts 里新增的八个用例从未在 CI 的 macOS 或 Windows 上执行过。考虑到新代码是文件系统形态的(readdir 探测、extension 目录扫描、store 路径里的 mkdir/chmod),而作者报告只在本地 macOS 上测过,Windows 目前无人覆盖。

(CI 表格见上方机器可读区域,中文不再重复。)

未验证项及原因:

  • 未验证:任何实时行为。 本次运行没有启动 daemon,也没有观测到任何 HTTP 响应。上文所有行为性结论均来自阅读源码。
  • 未验证:冷启动响应在本 PR 自己的 base build 上确实发生变化。 "Before" 证据针对的是全局安装的 0.18.5-preview.0 二进制 —— 一个不同的、旧得多的构建 —— 所以它证明的是该缺口曾经存在,而不是它在本 PR 的 merge base 上存在。缺的正是与 main 的 A/B 那一半。
  • 未验证:"没有 child 进程、没有 MCP、没有 hook 执行"。 我通过阅读确认了这条代码路径无法执行 extension 代码,这一点很有力。但作者的进程检查与 sentinel 检查是作者在其 macOS harness 上的陈述,此处未独立复现。
  • 未验证:非英文 locale 行为。 即上文第 1 条;没有测试覆盖 locale map 形式的 displayName

作者在本线程中的 E2E 报告(五个隔离 daemon 场景、163 个定向测试、preheat.status=not_scheduled、零会话零 ACP child)是作者的陈述,按此归属 —— 它不是本次审查复现的证据,也不计入上面的测试结论。

沙箱化验证可以定案:@qwen-code /verify —— "在 base build 上,冷启动的 daemon 本地响应从不含 extension 变为包含 active 加 inactive_extension 条目"这件事无法从 diff 观测到,而该主张的 "Before" 一半目前依赖一个无关的 0.18.5-preview.0 二进制,而不是与 main 的 A/B。一次承重运行还能定案"未 spawn child、未触发 MCP 或 hook sentinel",而这正是整个设计所依赖的安全属性 —— 静态阅读可以支持它,但无法证明它。作者具备 write 权限,因此 /verify 无需担保人;如果维护者更希望直接看到 Web Shell 首次 prompt 之前的 Skill 自动补全,@qwen-code /tmux 是对应的通道。

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the shape is right and the gating is more careful than this diff had to be, but it ships extension display names in the wrong language and quietly changes what a public daemon route does on a read. Both need a human's call rather than mine.

Stepping back: I like this PR. Ninety-four production lines, no drive-by churn, a design doc written before the code, and it deliberately mirrors the ACP child's producer instead of inventing a parallel mapping — which is the right instinct, because runtime/workspace-skills-mapping.ts only earns its keep if both sides actually converge. The trust gating is the part I'd single out as better than required: untrusted, inert-untrusted and safe-mode providers never construct an ExtensionManager at all, so there's no path where an untrusted workspace pulls extension runtime context in. I went looking for a way this executes extension code and didn't find one.

It also narrowed where the plan didn't. #11274 wanted the daemon-local provider to become the sole list authority and drop child preference; the direction review on that issue called the invariant flip a maintainer-level call. This PR keeps "the live child, when present, stays authoritative" intact and only fills the extension hole underneath it. That's the correct way to stage it, and it's why I'm at 3 rather than 2.

What keeps me from approving is that the one concrete defect sits on a surface the tests structurally cannot cover. Extension displayName and description are locale-resolved fields, and this path resolves them as English for everyone, while the child and the daemon's own extension routes both resolve the user's configured language. Every test manifest uses a plain-string display name, so the suite is green and the bug is invisible to it. For a repo with a large general.language: "zh" audience and an i18n layer built specifically for this field, that's not a nit — it's the feature arriving wrong for a meaningful slice of users, and then self-correcting confusingly once a session exists. It's a few lines to fix, and the fix is reuse rather than new code.

The other two are judgement calls I shouldn't make alone. A route that used to be a pure filesystem read now takes a cross-process lock, creates directories under QWEN_HOME, and can write state.json — all pre-existing behaviour of the store, but new to this endpoint, and the PR body frames the lock as a consistency guarantee rather than as a latency change to a path whose whole reason to exist is answering instantly. And the process-global cache this route reads has no invalidation hook for extension mutations, so /workspace/config/skills can serve a present-but-stale extension list where before this PR it served no extension list at all. #11274 assigns that hook to a later stage, which is a reasonable place for it — but "reasonable" and "decided by a maintainer" aren't the same thing, and the tracking issue is self-authored, still need-discussion, still P3, with no sign-off in the thread.

On evidence: CI is green on this commit with zero failures, one check (web-shell E2E Smoke) still running. The unit suite ran on Linux only — macOS and Windows Test jobs are skipped by matrix rules — and the author tested locally on macOS, so Windows has had no coverage of the new filesystem-shaped code from anyone. The behavioural claims (cold response now carries extension entries; no child spawned; no MCP or hook sentinel fires) rest on the author's harness, and the "Before" half is against an unrelated 0.18.5-preview.0 binary rather than this PR's merge base. Static reading supports all of it; nothing here proves it. @qwen-code /verify would, and the author has write access so it needs no sponsor.

One sequencing note for whoever picks this up: #10991 (refactor(daemon): decouple extension activation refresh) is a sibling in the same family and is also open. The direction review on #11274 already advised the two be sequenced consciously rather than merged in parallel.

Not requesting changes — nothing I found is a correctness blocker or a regression, and the code quality is high. Deferring because a public daemon contract changed, one defect needs fixing first, and two design questions belong to a maintainer.

中文说明

Confidence: 3/5 —— 形态是对的,门控比这个 diff 所要求的更细致,但它会以错误的语言返回 extension 展示名,并且悄悄改变了一条公开 daemon 路由在"读"时的行为。这两点需要人来定,而不是我。

退一步看:我喜欢这个 PR。九十四行生产代码,没有顺手改动,先写 design doc 再写代码,并且刻意对齐 ACP child 的 producer 而不是另造一套映射 —— 这是正确的直觉,因为 runtime/workspace-skills-mapping.ts 只有在两侧真正收敛时才有存在价值。信任门控是我要单独点名"做得比要求更好"的部分:untrusted、inert-untrusted 与 safe mode 三种 provider 根本不会构造 ExtensionManager,所以不存在任何路径让不可信 workspace 拉入 extension 运行时上下文。我专门找过"这里会不会执行 extension 代码",没找到。

它也做了计划本身没做的收窄。#11274 想让 daemon 本地 provider 成为列表的唯一来源并移除 child 优先;该 issue 的方向性 review 把翻转该不变量称为维护者级别的决定。本 PR 保留了"live child 存在时仍是权威",只填补其下的 extension 空缺。这是正确的分阶段方式,也是我给 3 分而不是 2 分的原因。

让我不去 approve 的是:那个具体缺陷正好落在测试结构上无法覆盖的面上。extension 的 displayNamedescription 是按 locale 解析的字段,而这条路径对所有人都解析成英文,与此同时 child 和 daemon 自己的 extension 路由都会解析用户配置的语言。每个测试 manifest 用的都是普通字符串展示名,所以测试全绿而这个 bug 对它是不可见的。对一个拥有大量 general.language: "zh" 用户、并且专门为此字段建立了 i18n 层的仓库来说,这不是小瑕疵 —— 这是功能对相当一部分用户以错误形态到达,然后在会话存在之后又以令人困惑的方式自我修正。修复只需几行,而且是复用而非新写代码。

另外两点是我不应独自做出的判断。一条此前是纯文件系统读取的路由,现在会取跨进程锁、在 QWEN_HOME 下创建目录、并可能写出 state.json —— 这些都是 store 的既有行为,但对这个端点是新的,而 PR 描述把这把锁写成一致性保证,而没有写成对一条"存在理由就是即时应答"的路径引入的延迟变化。以及,这条路由读取的进程级缓存对 extension 变更没有失效钩子,所以 /workspace/config/skills 可能返回一份"存在但陈旧"的 extension 列表 —— 而在本 PR 之前它根本不返回 extension 列表。#11274 把这个钩子安排到后续阶段,这是合理的归属 —— 但"合理"和"由维护者决定"不是一回事,而该 tracking issue 是自拟的,仍是 need-discussion、仍是 P3,线程里没有签字。

关于证据:该 commit 上 CI 全绿、零失败,还有一个检查(web-shell E2E Smoke)在运行。单元测试仅在 Linux 上跑过 —— macOS 与 Windows 的 Test job 被矩阵规则跳过 —— 而作者只在本地 macOS 上测过,因此新增的文件系统形态代码在 Windows 上没有任何人覆盖过。行为性主张(冷启动响应现在带有 extension 条目;未 spawn child;未触发 MCP 或 hook sentinel)依赖作者的 harness,而 "Before" 那一半针对的是一个无关的 0.18.5-preview.0 二进制,而不是本 PR 的 merge base。静态阅读支持这一切;但这里没有任何东西证明了它。@qwen-code /verify 可以证明,而作者具备 write 权限,无需担保人。

给接手的人一条排期提示:#10991refactor(daemon): decouple extension activation refresh)是同一家族的兄弟 PR,也处于 open。#11274 的方向性 review 已经建议两者有意识地排期,而不是并行合入。

不提交 request changes —— 我发现的问题没有一条是正确性阻塞或回归,代码质量很高。选择 defer,是因为一条公开 daemon 契约发生了变化、有一个缺陷需要先修,以及两个设计问题属于维护者。

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to a maintainer — not approving, not requesting changes.

I tried to resolve an accountable owner deterministically and came up empty: no QWEN_MAINTAINER_HANDLE in this run, the PR carries no labels so the area-owner map has nothing to match on, and there are no human reviews yet to fall back to. So there's no handle to @mention and nothing to assign — picking a login by eye is exactly what the resolver exists to avoid. A maintainer needs to pick this up from the thread.

What I can't settle from the diff, the tests and the PR description:

  1. Is the English-only extensionDisplayName acceptable to ship, or a fix-before-merge? It's a real wrong-output defect on a public daemon route, the fix is a few lines reusing resolveExtensionLocale, and no test can catch it as written. I'd fix it first, but that's the author's and maintainer's call, not a blocker I should assert.
  2. Should a previously read-only route now take the extension-store lock, create directories under QWEN_HOME, and potentially write state.json? All pre-existing store behaviour, new to this endpoint. Specifically: when the lock is contended, is blanking the entire catalog (initialized: false, skills: [], after up to ~25 s of retries) the intended failure semantics, or should the extension level degrade alone?
  3. Which stage owns the invalidation hook for /workspace/config/skills? Extension mutations invalidate the per-runtime provider but nothing invalidates the process-global pair this route reads, so it can serve a present-but-stale extension list. tracking(daemon): Decouple Skill management from child and refresh explicit sessions in PRs under 1000 lines #11274 defers invalidation policy to a later stage — I just want that deferral to be a decision, since this PR is what makes the stale state reachable.

Also worth a maintainer's eye: the tracking issue #11274 is self-authored, still need-discussion and P3 with no sign-off in the thread, and this PR changes the payload of two public daemon routes under it. And #10991 is an open sibling in the same decoupling family — the earlier direction review advised sequencing the two consciously rather than merging in parallel.

Full reasoning and the CI evidence are in the stage comments above. @qwen-code /verify would close the behavioural gap if someone wants proof rather than static reading.

中文说明

⏸️ 转交维护者 —— 不 approve,也不 request changes。

我尝试用确定性方式解析出一个可问责的 owner,结果为空:本次运行没有 QWEN_MAINTAINER_HANDLE,PR 没有任何标签,因此 area-owner 映射无从匹配,也还没有人类 review 可供回退。所以没有可 @mention 的账号,也没有可指派的对象 —— 凭眼力挑一个 login 正是解析器要避免的事。需要维护者从本线程接手。

我无法从 diff、测试和 PR 描述中定案的三点:

  1. 只返回英文的 extensionDisplayName 是可以合入,还是应先修? 这是公开 daemon 路由上一个真实的错误输出缺陷,修复只需几行并复用 resolveExtensionLocale,而且按现状没有任何测试能抓到它。我倾向先修,但这是作者与维护者的决定,不该由我断言为阻塞项。
  2. 一条此前只读的路由,现在是否应当取 extension store 锁、在 QWEN_HOME 下创建目录、并可能写出 state.json 这些都是 store 的既有行为,但对这个端点是新的。具体地说:当锁被占用时,清空整份目录(initialized: false, skills: [],且最多重试约 25 秒)是预期的失败语义,还是应当只让 extension 层单独降级?
  3. /workspace/config/skills 的失效钩子由哪个阶段负责? extension 变更失效的是 per-runtime provider,而这条路由读取的那对进程级单例无人失效,因此它可能返回一份"存在但陈旧"的 extension 列表。tracking(daemon): Decouple Skill management from child and refresh explicit sessions in PRs under 1000 lines #11274 把失效策略推迟到后续阶段 —— 我只是希望这个推迟是一个明确决定,因为正是本 PR 让这种陈旧状态变得可达。

另外值得维护者留意:tracking issue #11274 是自拟的,仍是 need-discussion 与 P3,线程中没有签字,而本 PR 在其下改变了两条公开 daemon 路由的返回内容。以及 #10991 是同一家族中处于 open 状态的兄弟 PR —— 早前的方向性 review 已建议两者有意识地排期,而不是并行合入。

完整推理与 CI 证据见上方的各阶段评论。如果有人想要证据而非静态阅读,@qwen-code /verify 可以弥合行为层面的空缺。

Qwen Code · qwen3.8-max-2026-09-02

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

samuelhsin
samuelhsin previously approved these changes Sep 7, 2026

@samuelhsin samuelhsin 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 at bb28cd1 — read the diff plus the surrounding code at head (ExtensionManager, ExtensionStore, SkillManager.collectCachedSkills, mapSkillConfigToStatus, resolveSkillSettings, and the sibling workspace-extensions-controller). Approving; the findings below are non-blocking follow-ups.

What I verified myself

  • Gating holds: the ExtensionManager is only constructed when workspaceTrusted && !safeMode && !disabledLevels.has('extension'), so safe/untrusted/inert-untrusted/disabled-level providers never touch extension state (and the tests spy that refreshCache/refreshTools are not called). Nothing on this path executes extension code.
  • The fs.readdir(Storage.getUserExtensionsDir()) precheck reads the same directory the store defaults to (ExtensionStore.extensionsDir ??= Storage.getUserExtensionsDir()), and it correctly distinguishes ENOENT (tolerated; loadExtensionsFromExtensionsDir returns []) from ENOTDIR/EACCES (thrown → uninitialized error status), preserving the pre-existing directory-failure semantics.
  • The enabled computation matches the mapper/settings contracts: enabledNames is trim+lowercased by normalizeSkillNames, hard disablements take precedence in mapSkillConfigToStatus, and state.workspaceEnabled ?? state.defaultEnabled matches getExtensionSkillState's {defaultEnabled, workspaceEnabled: boolean|null} shape. getExtensionSkillState's throw-on-foreign-skill is unreachable here because SkillManager sources extension skills from the same getActiveExtensions() objects.
  • The final statuses.sort(localeCompare) is required to merge the appended inactive entries into SkillManager's own name-sorted order, and V8's stable sort keeps a project entry ahead of a same-name inactive-extension entry — which the precedence test asserts.
  • Failed refreshCache cannot poison the cache: managers.set is only reached after every await succeeds, and the regression test covers the retry.

Follow-up 1 (real, small): locale is never passed, so extensionDisplayName/description resolve as English on this route. new ExtensionManager({workspaceDir, isWorkspaceTrusted}) falls back to locale: 'en' (extensionManager.ts:521), which is what resolveExtensionConfigLocale uses at manifest load (extensionManager.ts:1854). Every other producer passes one — the daemon's own extension routes use resolveExtensionLocale(workspaceDir, workspaceTrusted) reading general.language (workspace-extensions-controller.ts:317). So a general.language: "zh" user with a locale-mapped extension sees English names in the pre-child window, then the name flips language once the child answers. Not a regression (pre-PR these skills were absent entirely) and the window is short, but the fix is a few lines — settings is already loaded in buildWorkspaceSkillsStatus, so settings.merged.general?.language (or exporting/reusing resolveExtensionLocale) is in hand. Current tests can't catch it because every fixture uses a plain-string displayName.

Follow-up 2 (judgement, worth a comment in the code): store-busy degrades the whole catalog, slowly. refreshCachereadConsistent takes the proper-lockfile lock with retries: 60, factor: 1.2, 50–500ms (~25s budget) before throwing ExtensionStoreBusyError (extension-store.ts:1479-1494), and the provider's outer catch turns that into initialized: false, skills: [] — blanking project/user/bundled skills too, on a route whose reason to exist is answering instantly. Contention requires a concurrent extension mutation and this only runs on the cold per-workspace call, so it's rare; but consider either a shorter retry budget here or degrading the extension level alone (fall back to getActiveExtensions: () => [], i.e. exactly the pre-PR behavior) in a follow-up.

Noted, fine as staged: the process-global cache has no invalidation hook for extension mutations (deferred to a later stage of #11274 — acceptable since pre-PR this route served no extension list, so "present but stale" is still an improvement), and cold reads may create store directories / write state.json — pre-existing ExtensionStore behavior, new to this endpoint, worth one sentence in the design doc.

The staging itself is right: keeping "the live child stays authoritative" intact and only filling the extension hole underneath it is the correct way to slice #11274.

@callmeYe
callmeYe requested a review from wenshao September 7, 2026 09:20

@ytahdn ytahdn 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.

本 PR 主要做了什么

给 daemon 本地(子进程起来之前)的 /workspace/skills 兜底枚举补上 extension Skill 这块空缺:之前 getActiveExtensions 被 shim 成 () => [],所以没有子进程快照时,已安装 extension 的 Skill 永远不会出现在这份列表里。本 PR 改为从持久化 extension store 构造一个「不绑定运行时 Config」的 ExtensionManager,把 active extension 喂给 SkillManager(保留 project > user > extension > bundled 优先级),把 inactive extension 的 Skill 以 inactive_extension 状态作为管理项追加,并给整份列表加上按名字排序,从而和子进程侧 producer 的形态对齐。映射继续复用共享的 mapSkillConfigToStatus

What this PR does

Fills the extension-Skill gap in the daemon-local pre-child /workspace/skills fallback. Previously getActiveExtensions was shimmed to () => [], so installed extension Skills never surfaced until a session existed. It now builds an unbound ExtensionManager from the persistent store, feeds active extensions into SkillManager (preserving project > user > extension > bundled precedence), appends inactive-extension Skills as inactive_extension management entries, and sorts the whole list by name to match the child producer. Shape stays shared via mapSkillConfigToStatus.

结论 / Verdict:范围收敛、形态对齐、测试覆盖扎实,无 Critical。有 1 个 Important(display 层面的 locale 漂移)+ 若干 nit,故以 COMMENT 发布。


Important

I-1 · 本地枚举的 ExtensionManager 没有传 locale,非英文工作区会出现显示名漂移 / daemon-local ExtensionManager omits locale, causing a display-name flip on non-English workspaces.

workspace-skills-status.ts:147new ExtensionManager({ workspaceDir, isWorkspaceTrusted }) 未传 locale,于是 extensionManager.ts:521 回落成 this.locale = 'en'。而 i18n.tsresolveExtensionConfigLocale(rawConfig, this.locale) 会按 locale 解析 extension 的 displayName / description。子进程侧 producer 用的是 Config 自带的 manager(config.ts:2939 传了 locale: params.locale),daemon 里规范的工厂 workspace-extensions-controller.ts:315-317 也传了 resolveExtensionLocale(...)

后果:如果工作区语言非英文、且 extension manifest 写了本地化 displayName/description 对象,那么「首 prompt 之前」这份兜底列表会显示英文扩展名,一旦有子进程接管又翻成本地化名字。影响面只限「manifest 做了本地化」的 extension,属于纯显示问题,但这恰好是本次 design doc 立下的「两份列表不能漂移」这条不变量要防的点。

建议:直接复用 createExtensionManager / resolveExtensionLocale 拿到 per-workspace locale(顺带把 consent/setting 桩统一到一处),而不是在这里裸构造。

The daemon-local manager defaults locale to 'en' (extensionManager.ts:521), while the child uses the workspace locale (config.ts:2939) and the canonical controller resolves it via resolveExtensionLocale(...) (workspace-extensions-controller.ts:315-317). For a non-English workspace whose extensions ship localized displayName/description manifests, the pre-child catalog shows English names, then flips to localized once a child takes over. Display-only and scoped to localized manifests, but it is precisely the "two listings can't drift" invariant the design doc sets out to protect. Prefer reusing createExtensionManager / resolveExtensionLocale.

Nits

  • N-1 活跃 extension Skill 的 enabled 是手写复刻 Config.isSkillEnabled!state || enabledNames.has(...) || (workspaceEnabled ?? defaultEnabled))。当前与 config.ts:6211 等价,但将来 isSkillEnabled 新增禁用来源时不会自动同步到兜底路径。可考虑像 mapSkillConfigToStatus 那样抽一个共享判定,避免"启用/禁用"决策也漂移。
  • N-2 getExtensionSkillState 在 skill 名不属于该 extension 时会 throw;因为它在外层 try 内,单行不匹配会把整份列表退化成 initialized:false。概率很低(skills 本就来自同一批 extension),加个 per-row 兜底更稳。
  • N-3 缓存的 extensionManager 把 store snapshot(workspace override)冻结到 invalidate,而 settings.skills.enabled/disabled 每次都重读——同一份响应里两个不同的时效时钟。和"best-effort 兜底"注释一致,补一行说明即可。
  • N-4 enabledExtensionOverrides--enable-extension 启动覆盖)这里不像 Config 那样透传;daemon 子进程一般也没有它,如属有意可在 doc 的 out-of-scope 里点一句。

值得肯定 / Positives

  • 对 extensions 目录的 fs.readdir 预探测把 ENOTDIR 变成 error 状态、同时容忍 ENOENT,符合 design doc「目录失败返回未初始化错误」的要求。
  • safe mode / 未信任 / disabledLevels:['extension'] 的门控正确,并用 it.each 做了矩阵覆盖。
  • inactive 父 extension 的 Skill 一律 disabled:true,settings 里的 opt-in 无法偷偷激活它——正确。
  • 新增的整表按名排序让本地枚举与子进程对齐,也解释了那两处被调序的既有断言(这是想要的 parity 变化,不是 ci-bot 说的"mock 假象")。

发布时 CI 全绿(Test / Lint & Static / Serve A/B / Integration / web-shell E2E Smoke 均 pass),head 为 bb28cd1f

@callmeYe

callmeYe commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the locale finding from the reviews by ytahdn and samuelhsin. It affects the new extension metadata returned by this PR, so the fix stays within stage 2's enumeration scope.

The daemon-local provider now uses the existing language-setting resolver and existing locale resolver when constructing its unbound extension manager. This preserves configured-language aliases, QWEN_CODE_LANG precedence and auto detection without changing process-global UI language. The locale resolver's implementation is unchanged; it is now exported for this caller instead of duplicating the controller's resolution chain or importing the controller into the provider.

The regression was reproduced over real cold HTTP against the previous PR bundle: both active and inactive extension names were English under general.language=zh on /workspace/skills and /workspace/config/skills. The new parameterized regressions also failed before the fix.

Scope dispositions: shared enablement refactoring, speculative per-row exception recovery and startup extension-override propagation are not included. Cache expiry/invalidation and store contention policy remain the explicitly deferred stages/design questions of #11274. The current mapping reads Skills from the very same extension objects, so a foreign-skill fallback is not needed for this change.

Fixed in d90e39c50a030167b583e29aca39aca87178f843. Validation: full build, typecheck and bundle passed; 179 focused tests passed; changed-file ESLint and Prettier passed; two complete clean diff audits. Real HTTP verification now returns the expected Chinese active/inactive display names on both cold catalog routes. All five original HTTP scenarios also passed again, retaining zero sessions/ACP children and no MCP/hook execution.

The PR remains 4 files, 440 additions + 31 deletions = 471 lines total. No unrelated existing issues were fixed.

@callmeYe

callmeYe commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI follow-up for unchanged head d90e39c50a030167b583e29aca39aca87178f843:

The first Smoke failure was a GitHub HTTPS timeout during checkout, before tests ran. Its rerun reached Playwright but 10 cases failed to find UI elements. The retained trace explains the latter failure: Chromium reports widespread net::ERR_NETWORK_CHANGED resource failures, and the page snapshot shows the bootstrap fallback: A required resource failed to load ... /main.tsx. The split-persist retry trace alone contains 127 network-change errors; the same signature appears across the other failing scenarios.

The exact first failing split-persist case passes locally on the current head (1 passed). This smoke harness starts Vite and a page-routed mock daemon; it does not execute the changed CLI provider. Web Shell, SDK sources and the lockfile are unchanged relative to this PR's base.

Requested another rerun of the failed Smoke job. No product, test or workflow changes are included: runner/network diagnosis is recorded separately from the PR implementation and does not widen its scope.

Rerun result (2026-09-07 12:29 UTC): the unchanged head d90e39c50a030167b583e29aca39aca87178f843 passed the full Smoke job: 50 browser tests passed, plus the 5-test transcript document gate. The first previously failing split-persist test also passed against both current and exact baseline frontend/SDK sources using identical dependencies (8.8s / 7.6s locally). No source or test changes were needed. Successful job.

@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.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": locating DEFAULT_EXCLUDED_ENV_VARS ' definition to confirm whether a workspace env overlay may set QWEN_HOME (the trigger condition for the finding above).; "agent reverse-audit (round 1)": walking every refreshCache() / loadExtension throw site for a message that embeds an extension source URL, which would make the unredacted writeStderrLine a…; "agent reverse-audit (round 1)": enumerating all Storage "pinned runtime context" call sites (storage.ts:169-190) to check whether any daemon route scopes the extension store per runtime by a…; "agent 1a": whether the daemon's ACP child spawn forwards -e / --extensions (the trigger for finding 3) — I read config.ts:2315 but not the spawn-args builder.; "agent 1a": whether resolveSupportedLanguage() normalization in the new resolveLanguage(resolveLanguageSetting(...)) path can yield a different locale than the child's …, and 1 more.

中文说明

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)"locating DEFAULT_EXCLUDED_ENV_VARS ' definition to confirm whether a workspace env overlay may set QWEN_HOME (the trigger condition for the finding above)."agent reverse-audit (round 1)"walking every refreshCache() / loadExtension throw site for a message that embeds an extension source URL, which would make the unredacted writeStderrLine a…"agent reverse-audit (round 1)"enumerating all Storage "pinned runtime context" call sites (storage.ts:169-190) to check whether any daemon route scopes the extension store per runtime by a…"agent 1a"whether the daemon's ACP child spawn forwards -e / --extensions (the trigger for finding 3) — I read config.ts:2315 but not the spawn-args builder."agent 1a"whether resolveSupportedLanguage() normalization in the new resolveLanguage(resolveLanguageSetting(...)) path can yield a different locale than the child's …,另有 1 条。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts
Comment thread packages/cli/src/serve/workspace-skills-status.test.ts
@wenshao

wenshao commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Sep 7, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@qwen-code-dev-bot

qwen-code-dev-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

AutoFix round 2 finishedview run. See this round's report below.

中文说明

AutoFix 第 2 轮已完成 —— 查看运行。本轮报告见下方。

…11281)

- Give the extension load its own failure domain: a fault inside the
  store load degrades only extension entries (logged, not cached, so
  the next read retries), while an unreadable extensions root keeps
  the documented all-or-nothing error status.
- Resolve the extension locale on every call and key the cache on it,
  so a language change rebuilds instead of serving a frozen locale;
  guard a non-string general.language from throwing in resolution.
- Guard cache installs with a per-workspace invalidation epoch so an
  invalidate() delivered mid-build cannot be undone by that build, and
  coalesce concurrent cold builds.
- Gate skills.disabledLevels at discovery only; inactive-extension
  management entries still appear, matching the child producer.
- Pin the readdir guard with an error-identity assertion and a
  searchable-but-unlistable root case; extract assertReadableDir.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下:

Address review round — PR #11281

Base: main · Head before round: d90e39c50a · This round: 813893bb26 · Conflict resolution: none requested (--conflict false), no merge performed.

Feedback dispositions

Review rv:5130485638 (@ytahdn, COMMENTED)

  • I-1 (Important) — daemon-local ExtensionManager omitted locale: already resolved. Fixed by d90e39c50a (head before this round); the parameterized locale it.each arms pin en/zh/zh-CN/auto resolution and QWEN_CODE_LANG precedence. No change needed this round.
  • N-1 — hand-rolled enabled judgment replicates Config.isSkillEnabled: deferred. Sharing the decision means extracting it from core config.ts, which is outside this PR's footprint. Recorded in the design doc's later-stage items and the deferred-findings queue.
  • N-2 — getExtensionSkillState throws on a foreign skill name; add per-row fallback: declined with evidence. The throw is unreachable in this data flow: extension Skills returned by SkillManager.listSkills() are discovered from the same getLoadedExtensions() objects the lookup uses, so extension.skills always contains the name (extensionManager.ts:783-790 throws only when it does not), and the statuses loop only calls it when extensions.find matched. A per-row catch would mask an invariant violation that should surface whole.
  • N-3 — cached store snapshot vs per-call settings re-read: addressed. One comment added at the resolveSkillSettings call site documenting the two deliberate freshness clocks.
  • N-4 — enabledExtensionOverrides not propagated: addressed. Recorded as out of scope in the design doc.

Review rv:5132529744 (@qwen-code-ci-bot, CHANGES_REQUESTED) — inline findings

  • rc:3950188470 (R1-1, Critical) — extension load has no failure domain: fixed. Manager construction + refreshCache() now sit in their own try/catch: on failure the provider logs via writeStderrLine, serves an initialized catalog with the project/user/bundled Skills and no errors cell (so skill delete does not 503), leaves extensionManager undefined, and does not cache the degraded pair so the next read retries. The finding's open tension was decided deliberately and recorded in the design doc: the extensions-root readdir probe stays outside the inner failure domain, so an unreadable root keeps the documented all-or-nothing initialized: false status, while faults inside the load degrade alone. The old does not cache a failed store read… test asserted the opposite contract and was deliberately rewritten in the same commit.
  • rc:3950188483 (R1-2, Critical) — locale frozen into the cached manager: fixed. The locale is resolved on every call from that call's freshly loaded settings, carried on the cache entry, and a mismatch is treated as a miss (entry deleted and rebuilt — locale is private readonly, so mutation is impossible). Rebuild fires only on an actual locale change; the SkillManager-reuse test still pins instance identity across same-locale calls.
  • rc:3950188489 (R1-3, Critical) — as string | undefined cast lets a non-string general.language throw: fixed. A typeof guard replaces the cast; QWEN_CODE_LANG || setting || 'auto' precedence is unchanged, and no global coercion of the setting was introduced. Only this call site is fixed; the sibling cast family (initializer.ts, llm.tsx, update.ts, extensions/list.ts, config.ts, workspace-extensions-controller.ts) is follow-up scope per the finding's own note.
  • rc:3950188496 (R1-4, Critical) — mid-build invalidate() swallowed: fixed. A per-workspace epoch map is bumped synchronously inside invalidate; the build captures the epoch before its first await and installs only while it is unchanged, so the in-flight pre-mutation snapshot is served to its own caller but never poisons the cache.
  • rc:3950188503 (R1-5, Suggestion) — locale source diverges from sibling route: deferred. This provider follows the documented daemon convention (skipLoadEnvironment; environment.ts:526-528 — per-workspace .env reaches the daemon only through the runtime spawn loop); the sibling /workspace/extensions route is the outlier leaking a workspace .env into process-global env. Dropping skipLoadEnvironment here would be the wrong fix (the finding says so itself); fixing the leak belongs to that route, outside this PR's footprint. Recorded in the design doc and the deferred-findings queue; thread left open with a reply.
  • rc:3950188511 (R1-6, Suggestion) — level gate also suppresses inactive_extension rows: fixed. The gate is split: the manager is built whenever workspaceTrusted && !safeMode, and the level check moved into the shim's getActiveExtensions, so discovery is gated while inactive management entries are appended unconditionally — matching the child producer (verified at acpAgent.ts:7231-7250: unconditional inactive loop, unconditional { disabled: true }, level:extensionName:name dedupe). The it.each 'disabled-level' arm was split into its own test asserting both halves.
  • rc:3950188523 (R1-7, Suggestion) — every cache miss pays a full locked reload; concurrent cold reads duplicate it: partially addressed. Concurrent cold builds of one workspace are now coalesced per provider instance, keyed by the invalidation epoch so a post-mutation caller never joins a pre-mutation build (pinned by a new test). The retain-across-invalidate() half is deferred: routing daemon invalidations through refreshCacheIfSourcesChanged would violate the documented "fingerprint is only the out-of-band safety net" rule unless invalidate carried provenance (extension-store mutation vs skill-settings mutation), and wiring that provenance crosses route/controller call sites outside this PR's footprint. Recorded in the design doc and the deferred-findings queue; thread left open with a reply.
  • rc:3950188527 (R1-8, Suggestion) — readdir guard load-bearing but unpinned: fixed. The guard stays; the existing regular-file case now asserts the error identity (ENOTDIR … scandir), and a new mode-0111 case (skipped on win32/root) asserts initialized: false with EACCES for a searchable-but-unlistable root. The tolerate-ENOENT/rethrow-else idiom is extracted into one assertReadableDir helper shared by both call sites. Deleting the probe line now turns two tests red (verified by probe).
  • rc:3950188533 (R1-9, Suggestion) — inactive-append/sort assembly duplicated with the child: deferred. The extraction's other half lives in acpAgent.ts, outside this PR's footprint, and the finding itself sanctions recording it in the design doc. Recorded (keeping the child's level:extensionName:name dedupe key) and queued as a deferred finding; thread left open with a reply.
  • rc:3950188544 (R1-10, Suggestion) — four unpinned enumeration paths: fixed. All four additions landed: (1) a refreshCache spy in the reuse test asserts exactly one store read across two calls; (2) uses persisted workspace activation… gains a manifest-default-ON skill with a workspace override of false, asserting disabled/default; (3) the active/inactive test now runs with skills.enabled: ['inactive-skill'] and keeps disabledReason: 'inactive_extension'; (4) a duplicate-name fixture asserts exactly one dup row. Each goes red under its named mutation (see probes below).

The review's "not explored to full depth" note lists unconfirmed reverse-audit threads; nothing actionable was asserted there, so there is nothing to reproduce.

Mutation probes (every guard/branch this round adds has a witness)

Each probe mutated the source, ran the focused test(s), confirmed failure, then restored and re-confirmed green:

Probe Mutation Result
R1-1 inner catch rethrows instead of degrading serves the non-extension catalog when the extension load fails… failed
R1-2 locale comparison dropped from the cache-hit condition re-resolves the extension locale… failed
R1-3 typeof guard reverted to the as cast ignores a non-string general.language… failed
R1-4 epoch guard dropped from the cache install does not install a pre-mutation snapshot… failed
R1-6 level gate moved back onto manager construction gates extension discovery but still lists inactive entries… failed
R1-7 in-flight coalescing disabled coalesces concurrent cold builds… failed
R1-8 extensions-root probe line deleted (full suite) both the ENOTDIR identity test and the 0111 test failed (2 red)
R1-10.1 every cache hit forced to rebuild reuses one SkillManager per workspace… failed
R1-10.2 ??|| at the workspace-override fallback uses persisted workspace activation… failed
R1-10.3 inactive loop's { disabled: true } made conditional on enabledNames lists active and inactive extension Skills… failed
R1-10.4 per-extension seenNames guard removed lists a duplicate Skill name… failed

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint (full repo) — passed
  • npx eslint packages/cli/src/serve/workspace-skills-status.ts packages/cli/src/serve/workspace-skills-status.test.ts — passed
  • npx prettier --check on the three changed files — passed (after one --write formatting pass)
  • npx vitest run src/serve/workspace-skills-status.test.ts (packages/cli) — 36/36 passed
  • npx vitest run src/serve/workspace-service/__tests__/facade.test.ts src/serve/routes/workspace-skills.test.ts (packages/cli) — 157/157 passed
  • 11 mutation probes — each named test went red under its mutation and green after restore (table above)
  • Pre-round regression check: the new/rewritten tests for R1-1, R1-2, R1-3, R1-4, R1-6 and the coalescing test fail against the pre-round head d90e39c50a by construction (each asserts behavior the old code did not have); R1-8/R1-10 additions pin pre-existing guards and pass in both states
  • npm run generate:settings-schema — not run: no settings source changed
  • Integration tests after npm run bundle — not run: the touched behavior is exercised directly by the focused Vitest suite (the provider is unit-testable without the bundled CLI), not only through the bundle or the integration harness
中文说明

本轮评审处理 — PR #11281

基线:main · 本轮前 head:d90e39c50a · 本轮提交:813893bb26 · 冲突处理:未要求(--conflict false),未执行合并。

反馈处置

评审 rv:5130485638(@ytahdn,COMMENTED)

  • I-1(Important)— 本地 ExtensionManager 未传 locale:已解决。 由本轮之前的 head d90e39c50a 修复;参数化的 locale it.each 用例已钉住 en/zh/zh-CN/auto 解析与 QWEN_CODE_LANG 优先级。本轮无需改动。
  • N-1 — 手写复刻 Config.isSkillEnabled 的启用判定:推迟。 共享该判定需要从 core 的 config.ts 抽取,超出本 PR 的改动范围。已记入设计文档的后续事项与推迟队列。
  • N-2 — getExtensionSkillState 对不属于该 extension 的 skill 名抛错,建议逐行兜底:带证据拒绝。 在当前数据流中该抛错不可达:SkillManager.listSkills() 返回的 extension Skill 正是从查找所用的同一批 getLoadedExtensions() 对象中发现的,因此 extension.skills 必然包含该名字(extensionManager.ts:783-790 仅在不包含时抛错),且 statuses 循环只在 extensions.find 命中时才调用。逐行 catch 只会掩盖本应整体暴露的不变量破坏。
  • N-3 — 缓存的 store 快照与每次重读的 settings 两个时效时钟:已处理。resolveSkillSettings 调用处加了一行注释,说明两个时效时钟是有意为之。
  • N-4 — enabledExtensionOverrides 未透传:已处理。 已在设计文档中记为范围之外。

评审 rv:5132529744(@qwen-code-ci-bot,CHANGES_REQUESTED)— 行内发现

  • rc:3950188470(R1-1,Critical)— extension 加载没有独立失败域:已修复。 manager 构造与 refreshCache() 现在有独立的 try/catch:失败时通过 writeStderrLine 记日志,返回一份 initialized、只含 project/user/bundled Skill、且不带 errors 字段的目录(因此删除 skill 不会 503),extensionManager 保持 undefined,并且缓存这次降级的组合,下一次读取会重试。该发现提出的张力已被刻意决策并写入设计文档:extensions 根目录的 readdir 预探测留在内层失败域之外,不可读根目录仍返回文档约定的「全有或全无」initialized: false 状态,而加载内部的故障单独降级。原有的 does not cache a failed store read… 测试断言的是相反的契约,已在同一提交中刻意重写。
  • rc:3950188483(R1-2,Critical)— locale 被冻结在缓存的 manager 里:已修复。 每次调用都基于该次新加载的 settings 解析 locale,存入缓存条目,不一致即视为未命中(删除并重建 —— localeprivate readonly,无法原地更新)。只有 locale 真的变化才重建;SkillManager 复用测试仍钉住同 locale 下的实例同一性。
  • rc:3950188489(R1-3,Critical)— as string | undefined 断言让非字符串 general.language 抛错:已修复。typeof 值保护替代断言;QWEN_CODE_LANG || setting || 'auto' 优先级不变,也没有引入全局的设置强转。按发现自身的范围说明,只修这一处调用点;其余同族断言(initializer.ts、llm.tsx、update.ts、extensions/list.ts、config.ts、workspace-extensions-controller.ts)留作后续。
  • rc:3950188496(R1-4,Critical)— 冷构造中途送达的 invalidate() 被吞:已修复。 新增按 workspace 的纪元(epoch)Map,在 invalidate 内同步自增;构造在第一个 await 之前捕获纪元,仅当纪元未变时才写入缓存 —— 在途的变更前快照仍返回给它自己的调用方,但不会再污染缓存。
  • rc:3950188503(R1-5,Suggestion)— locale 来源与兄弟路由分歧:推迟。 本 provider 遵循 daemon 的文档约定(skipLoadEnvironment;environment.ts:526-528 —— per-workspace .env 只通过 runtime spawn 循环进入 daemon);分歧的一方是把 workspace .env 泄漏进进程级 env 的 /workspace/extensions 兄弟路由。按发现自身的分析,在这里去掉 skipLoadEnvironment 是错误的修法;修复泄漏属于那条路由,超出本 PR 范围。已记入设计文档与推迟队列;该线程保留开放并回复说明。
  • rc:3950188511(R1-6,Suggestion)— 层级门控同时压制 inactive_extension 管理条目:已修复。 门控拆开:只要 workspaceTrusted && !safeMode 就构造 manager,层级判断移入 shim 的 getActiveExtensions,发现层被门控而 inactive 管理条目无条件追加 —— 与 child 生产者一致(已在 acpAgent.ts:7231-7250 核实:无条件 inactive 循环、无条件 { disabled: true }level:extensionName:name 去重键)。it.each 的 'disabled-level' 分支被拆成独立测试,同时断言两个半边。
  • rc:3950188523(R1-7,Suggestion)— 每次未命中都付出完整带锁重载、并发冷读重复:部分处理。 同一 workspace 的并发冷构造现在按 provider 实例合并,并以失效纪元为键,变更后的调用方不会并入变更前的构造(已有新测试钉住)。跨 invalidate() 保留 manager 的那一半推迟:让 daemon 触发的失效走 refreshCacheIfSourcesChanged,除非 invalidate 携带来源信息(extension store 变更 vs skill 设置变更),否则违反「指纹只是带外安全网」的文档规则,而打通该来源需要改动本 PR 范围之外的 routes/controllers 调用点。已记入设计文档与推迟队列;该线程保留开放并回复说明。
  • rc:3950188527(R1-8,Suggestion)— readdir 预探测有承重但没有测试钉住:已修复。 预探测保留;既有 regular-file 用例现在断言错误标识(ENOTDIR … scandir),并新增一个 mode-0111 用例(win32/root 下跳过),断言可进入但不可列目录的根返回 initialized: false 且错误为 EACCES。「容忍 ENOENT、其余 rethrow」的写法被抽成两个调用点共用的 assertReadableDir helper。删除该探测行现在会让两个测试变红(已用变异验证)。
  • rc:3950188533(R1-9,Suggestion)— inactive 追加/排序装配与 child 重复:推迟。 抽取的另一半在 acpAgent.ts(child 生产者),超出本 PR 范围,且发现本身认可记入设计文档的处置。已记录(保留 child 的 level:extensionName:name 去重键)并加入推迟队列;该线程保留开放并回复说明。
  • rc:3950188544(R1-10,Suggestion)— 四条未被测试区分的枚举路径:已修复。 四处补充全部落地:(1) 复用测试中新增 refreshCache spy,断言两次调用只读一次 store;(2) uses persisted workspace activation… 新增一个 manifest 默认开启、但 workspace 覆盖为 false 的 skill,断言 disabled/default;(3) active/inactive 测试现在在 skills.enabled: ['inactive-skill'] 下运行,仍保持 disabledReason: 'inactive_extension';(4) 新增同名重复 fixture,断言名为 dup 的只有一行。每一项都在其指名变异下变红(见下方变异验证表)。

评审中「未探索到全部深度」的说明只列出未确认的反向审计线索,没有可复现的断言,因此无需处理。

变异验证(本轮新增的每个守卫/分支都有见证)

每次变异都是:改动源码 → 运行对应测试确认失败 → 还原后确认全绿(见上方英文表格)。

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint(全仓库)— 通过
  • npx eslint packages/cli/src/serve/workspace-skills-status.ts packages/cli/src/serve/workspace-skills-status.test.ts — 通过
  • npx prettier --check(三个改动文件)— 通过(先经一次 --write 格式化)
  • npx vitest run src/serve/workspace-skills-status.test.ts(packages/cli)— 36/36 通过
  • npx vitest run src/serve/workspace-service/__tests__/facade.test.ts src/serve/routes/workspace-skills.test.ts(packages/cli)— 157/157 通过
  • 11 项变异验证 —— 每个指名测试在变异下变红、还原后变绿(见英文表格)
  • 变更前回归检查:R1-1、R1-2、R1-3、R1-4、R1-6 与并发合并测试按构造在本轮前 head d90e39c50a 上失败(各自断言旧代码不具备的行为);R1-8/R1-10 的补充钉住的是既有守卫,两种状态下都通过
  • npm run generate:settings-schema — 未运行:没有 settings 源变更
  • npm run bundle 后的集成测试 — 未运行:被改行为由聚焦的 Vitest 套件直接覆盖(provider 无需打包 CLI 即可单测),并非只能通过 bundle 或集成 harness 触达

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

🧵 Resolved all 7 selected review thread(s). · 已关闭全部选中的 7 条评审线程。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 kimi-k3 · CLI 0.23.0

@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.

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-5 sibling-route locale divergence over a workspace .env QWEN_CODE_LANG — already reported (comment 3950188503), author deferred and recorded in this round's design doc
  • R1-9 inactive-append and sort assembly duplicated from the child producer — already reported (comment 3950188533), author deferred and recorded in this round's design doc

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread docs/design/daemon-extension-skill-catalog.md Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.test.ts
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
…view (#11281)

- Decide directory absence by lstat and readability by readdir, so a
  dangling symlink at the extensions root fails closed instead of
  reading as absent and silently dropping every extension Skill.
- Degrade an unreadable extensions root for workspaces that disabled
  extension discovery: the catalog-fatal probe no longer empties a
  catalog that opted out of extension Skills.
- Stop latching daemon-local skills answers in the workspace facade
  (the latch stays child-only), so a degraded extension enumeration
  retries on the next poll instead of freezing the pre-child window.
- Drop the unreachable disabled-level arm from the active-extensions
  shim; SkillManager already applies that gate before the only call.
- Pin the degrade stderr line, the non-string language guard, and the
  superseded-build coalescing bookkeeping with discriminating tests;
  record the child locale-normalization divergence and the correct
  --extensions flag name in the design doc.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下:

Autofix round — PR #11281 review feedback

One commit: 284fcb464a fix(daemon): close skills-catalog probe, tier, and latch gaps from review (#11281) — 5 files, +244/−42. No base merge was needed (--conflict false).

Addressed (9 findings)

  • [Critical] R2-1 (rc:3953203532) — dangling-symlink extensions root read as "absent". assertReadableDir now decides absence by lstat (tolerating only its ENOENT) and readability by readdir, so a present-but-unresolvable root (dangling link) fails closed with the same uninitialized error status as the regular-file shape, instead of certifying an initialized catalog missing every extension Skill. New test fails closed for a dangling symlink at the extensions root (skipped on win32, symlink privileges, matching the existing EACCES case); design doc records the tier. Mutation probe: removing the lstat arm turns the test red (initialized: true, no errors cell).
  • [Critical] R1-6 (rc:3953203535) — disabled extension level exposed to the catalog-fatal root probe. The probe moved inside the load's try; only the probe stays catalog-fatal and only while discovery is enabled (if (!rootProbed && !extensionLevelDisabled) throw), so a workspace with skills.disabledLevels: ['extension'] degrades an unreadable root like any other load fault and keeps its project/user/bundled catalog (/review included), while the enabled-level all-or-nothing contract and the store read for inactive entries are unchanged. New test degrades an unreadable extensions root when the extension level is disabled. Mutation probe: dropping the demotion turns it red (initialized: false).
  • [Critical] R1-1 (rc:3953203539) — facade latches the degraded answer, retry never happens. The workspace facade no longer stores daemon-local answers in lastWorkspaceSkillsStatus (the latch stays child-only, as its comment always said); the provider is always callable and caches its own managers, so healthy repeats stay cheap while a degraded initialized: true answer retries on the next read past the TTL. New facade test getWorkspaceSkillsStatus retries a degraded daemon-local answer instead of latching it (degraded first, complete second, Date.now advanced 6s); the sibling fallback test's latch assertion was deliberately rewritten (toHaveBeenCalledOncetoHaveBeenCalledTimes(2) with the reason recorded in a comment). Mutation probe: restoring the store turns the new test red.
  • [Suggestion] R2-2 (rc:3953203548) — nonexistent flag in the design doc. --enable-extension--extensions (verified: the only repo hit for the former was that doc line; the real option is registered in top-level-options.ts).
  • [Suggestion] R2-3 (rc:3953203559) — non-string-language guard pinned only when QWEN_CODE_LANG is unset. The test now stubs QWEN_CODE_LANG to '' (env loses to settings, matching the it.each matrix convention). Mutation probe: replacing the typeof rawLanguage === 'string' guard with a bare rawLanguage goes red via initialized: false even with QWEN_CODE_LANG=en exported in the ambient environment.
  • [Suggestion] R2-9 (rc:3953203580) — degrade stderr line unasserted. The degrade-and-retry test now asserts exactly one stderr line containing store unavailable. Mutation probe: deleting the writeStderrLine call goes red.
  • [Suggestion] R2-10 (rc:3953203583) — unreachable disabledLevels.has('extension') arm. Deleted from the shim's getActiveExtensions; SkillManager.listSkillsAtLevel already returns [] at the level gate (skill-manager.ts:992) before the getter's only call site (:1011, verified as the sole getActiveExtensions consumer), and the shim supplies that same set via getDisabledSkillLevels. Per the finding this deletion is un-pinnable by design; the evidence is gates extension discovery but still lists inactive entries when the extension level is disabled green before and after, plus a probe run showing a throw injected into the deleted arm never fires.
  • [Suggestion] R2-4 (rc:3953203566) — superseded-build coalescing bookkeeping unwitnessed. New test starts a fresh build for post-invalidation callers and lets later callers join it: real refreshCache delegated per call and parked after the first build's store read, mutation + invalidate delivered mid-build, second caller asserted to build fresh (exactly 2 store reads), third caller asserted to join the fresh build (toBe), superseded build's clear() asserted not to evict the fresh entry. Mutation probes: both pending?.epoch === epochpending and dropping the clear() identity check go red.
  • [Suggestion] R2-5 (rc:3953203567) — daemon/child locale-normalization divergence. Applied the finding's prescribed in-scope disposition: recorded in the design doc's "Known later-stage items" (daemon normalizes general.language, child passes it through verbatim; shared-resolver extraction as the follow-up, preserving env-over-settings precedence). Doc-only per the finding; no new test required.

Deferred to the next round (4 findings, batch bound)

This round already implements 9 findings (3 Critical + 6 witness/doc items); per the ~8-finding batch bound the remaining Suggestions are deferred explicitly, not dropped — each has a reply on its own thread: R1-7 (rc:3953203563, coalesce the manager build rather than the whole response), R2-6 (rc:3953203569, disabledLevels cache-eviction key), R2-7 (rc:3953203576, split degraded cache + stderr dedupe), R2-8 (rc:3953203578, probe reorder — deliberately sequenced after this round's restructure of the same extension block).

Verification

  • npm run build — passed (run twice: once before tests, once after all source edits; second run clean).
  • npm run typecheck — passed.
  • npm run lint — passed (no errors; pre-commit lint-staged also ran on the commit).
  • npx vitest run src/serve/workspace-skills-status.test.ts src/serve/workspace-service/__tests__/facade.test.ts (packages/cli, env -u QWEN_CODE_LANG) — 172 passed (39 provider + 133 facade), re-confirmed on the exact committed tree.
  • Mutation probes (each applied, observed, then reverted; all restored files verified byte-identical to the committed state): P1 remove lstat arm → dangling-symlink test red; P2 remove disabled-level demotion → degrade test red; P3 restore facade latch → retry test red; P4 remove typeof guard → non-string-language test red even with ambient QWEN_CODE_LANG=en; P5 delete degrade writeStderrLine → assertion red; P6a weaken epoch join check → coalescing test red; P6b drop clear() identity check → coalescing test red.
  • Not run: integration tests (the touched behavior is exercised by the unit suites above, not only through the bundled CLI); settings-schema regeneration (no settings source changed). The review's disclosed gap stands: the windows-latest suite was skipped in CI, so the new POSIX-only dangling-symlink case (skipped on win32 like the existing EACCES case) has no Windows witness this round.
中文说明

Autofix 本轮处理 — PR #11281 审查反馈

一次提交:284fcb464a fix(daemon): close skills-catalog probe, tier, and latch gaps from review (#11281) —— 5 个文件,+244/−42。无需合并基线(--conflict false)。

已处理(9 条发现)

  • [Critical] R2-1(rc:3953203532)—— 悬空软链接的 extensions 根被当作「不存在」。 assertReadableDir 现在用 lstat 判断是否存在(只容忍它的 ENOENT)、用 readdir 判断是否可读,于是「存在但无法解析」的根(悬空链接)与普通文件形态一样 fail closed(未初始化错误状态),而不是在所有 extension Skill 静默缺失的情况下把目录认证为健康。新增用例 fails closed for a dangling symlink at the extensions root(在 win32 上跳过 —— 软链接权限,与现有 EACCES 用例一致);设计文档已记录该层级。变异探针:移除 lstat 分支后该用例变红(initialized: true、无 errors)。
  • [Critical] R1-6(rc:3953203535)—— 禁用 extension 层级的 workspace 暴露在给整份目录定生死的根探测之下。 探测移入加载的 try 内;只有探测保持 catalog-fatal,且仅在发现启用时(if (!rootProbed && !extensionLevelDisabled) throw),于是设置了 skills.disabledLevels: ['extension'] 的 workspace 对不可读根与其他加载故障一样走降级路径,保住 project/user/bundled 目录(含 /review);启用层级的「全有或全无」契约以及为 inactive 条目所做的 store 读取均不变。新增用例 degrades an unreadable extensions root when the extension level is disabled。变异探针:去掉降级处理后该用例变红(initialized: false)。
  • [Critical] R1-1(rc:3953203539)—— facade 锁存降级答案,重试永不发生。 workspace facade 不再把 daemon 本地答案存进 lastWorkspaceSkillsStatus(锁存保持仅对 child 答案,与其注释始终一致);provider 永远可调用且自带 manager 缓存,因此健康路径的重复读取仍然廉价,而降级为 initialized: true 的答案会在下一次超过 TTL 的读取时重试。新增 facade 用例 getWorkspaceSkillsStatus retries a degraded daemon-local answer instead of latching it(先降级、后完整,Date.now 前进 6 秒);同文件相邻回退用例的锁存断言被刻意改写(toHaveBeenCalledOncetoHaveBeenCalledTimes(2),理由写入注释)。变异探针:恢复锁存后新用例变红。
  • [Suggestion] R2-2(rc:3953203548)—— 设计文档写了不存在的 flag。 --enable-extension--extensions(已验证:前者在全仓库唯一的命中就是那句文档;真实选项注册于 top-level-options.ts)。
  • [Suggestion] R2-3(rc:3953203559)—— 非字符串 language 守卫只在 QWEN_CODE_LANG 未设置时被钉住。 该用例现在把 QWEN_CODE_LANG 打桩为 ''(环境变量让位于设置项,与 it.each 矩阵的约定一致)。变异探针:把 typeof rawLanguage === 'string' 守卫换成裸 rawLanguage 后,即使环境导出 QWEN_CODE_LANG=en,用例也因 initialized: false 变红。
  • [Suggestion] R2-9(rc:3953203580)—— 降级路径的 stderr 行无人断言。 「降级并重试」用例现在断言恰好一行 stderr、且包含 store unavailable。变异探针:删掉该 writeStderrLine 调用即变红。
  • [Suggestion] R2-10(rc:3953203583)—— 不可达的 disabledLevels.has('extension') 分支。 已从 shim 的 getActiveExtensions 删除;SkillManager.listSkillsAtLevel 在该 getter 唯一调用点(:1011,已验证为唯一消费方)之前就在层级门(skill-manager.ts:992)返回 [],且 shim 通过 getDisabledSkillLevels 提供的正是同一个集合。按发现所述,此删除在设计上无法被测试钉住;证据是 gates extension discovery but still lists inactive entries when the extension level is disabled 在删前删后均绿,外加一次探针运行:向被删分支注入 throw 后它从未触发。
  • [Suggestion] R2-4(rc:3953203566)—— 处理「被取代构造」的合并簿记没有见证。 新增用例 starts a fresh build for post-invalidation callers and lets later callers join it:逐次委托真实 refreshCache 并在首次构造的 store 读取之后门控,构造进行中提交变更并送达 invalidate,断言第二个调用方启动全新构造(恰好 2 次 store 读取)、第三个调用方加入新构造(toBe)、被取代构造的 clear() 不驱逐新条目。变异探针:pending?.epoch === epoch 弱化为 pending、以及去掉 clear() 的同一性判断,两者都变红。
  • [Suggestion] R2-5(rc:3953203567)—— daemon 与 child 的 locale 归一化分歧。 按发现给出的范围内处置:记入设计文档「Known later-stage items」(daemon 会归一化 general.language,child 原样透传;以抽取共享 resolver 为后续项,保持环境变量优先于设置项)。按发现为纯文档处置,无需新测试。

推迟到下一轮(4 条发现,受批次上限约束)

本轮已实现 9 条发现(3 条 Critical + 6 条见证/文档项);按约 8 条的批次上限,其余 Suggestion 明确推迟而非丢弃 —— 每条都在其原线程下收到回复:R1-7(rc:3953203563,合并 manager 构造而非整份响应)、R2-6(rc:3953203569,disabledLevels 缓存驱逐键)、R2-7(rc:3953203576,拆分降级缓存 + stderr 去重)、R2-8(rc:3953203578,探测重排 —— 刻意排在本轮对同一 extension 代码段的重构之后)。

验证

  • npm run build —— 通过(跑两次:测试前一次、全部源码修改后一次;第二次干净通过)。
  • npm run typecheck —— 通过。
  • npm run lint —— 通过(无错误;提交时 pre-commit 的 lint-staged 也已运行)。
  • npx vitest run src/serve/workspace-skills-status.test.ts src/serve/workspace-service/__tests__/facade.test.ts(packages/cli,env -u QWEN_CODE_LANG)—— 172 通过(provider 39 + facade 133),并在最终提交树上的状态复跑确认。
  • 变异探针(每次先应用、观察、再还原;所有还原后的文件均已验证与提交内容逐字节一致):P1 移除 lstat 分支 → 悬空软链接用例红;P2 移除禁用层级降级 → 降级用例红;P3 恢复 facade 锁存 → 重试用例红;P4 移除 typeof 守卫 → 非字符串 language 用例红(环境导出 QWEN_CODE_LANG=en 时依然红);P5 删除降级 writeStderrLine → 断言红;P6a 弱化 epoch 加入判断 → 合并用例红;P6b 去掉 clear() 同一性判断 → 合并用例红。
  • 未运行:集成测试(本次触及的行为由上述单元套件覆盖,并非只有打包后的 CLI 才能触达);settings schema 再生成(未改动任何 settings 源)。审查披露的缺口仍然存在:CI 跳过了 windows-latest 套件,因此新增的 POSIX 限定悬空软链接用例(与现有 EACCES 用例一样在 win32 跳过)本轮没有 Windows 见证。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

🧵 Resolved all 9 selected review thread(s). · 已关闭全部选中的 9 条评审线程。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 kimi-k3 · CLI 0.23.0

@callmeYe callmeYe removed the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Sep 8, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply autofix/takeover (or comment @qwen-code /takeover) to re-engage.

中文说明

👋 已释放:autofix 循环不再介入此 PR(在飞的一轮如有,将完成其有界工作)。重新打上 autofix/takeover 标签(或评论 @qwen-code /takeover)即可再次接管。

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下:

Autofix round — PR #11281 (address-review)

Commit: cf66404d03docs(daemon): record skills-catalog read contract and later-stage items (#11281) (docs-only, +29/-5; PR total stays ~735/1000 lines).

This round deliberately implements the smallest blocking subset (budget warning noted): the two in-footprint documentation Suggestions, plus the documentation alternative the round-4 Critical itself offers. The three contested Criticals are escalated or declined with evidence, not silently dropped.

Feedback dispositions

Resolved in an earlier commit, re-verified at this head

  • R3-2 (rc:3955145195, Suggestion) — absent-root store creation. Fixed at 8d5907ed26: workspace-skills-status.ts gates manager construction on the lstat probe (if (entry)), and does not create an extension store when no extensions directory exists pins that refreshCache is not called and neither extensions/ nor extension-store/ is created. Verified green in this round's suite run.
  • R3-3 (rc:3955145205, Suggestion)_rawLocalizable private-field reach. Fixed at 8d5907ed26: no _rawLocalizable reference remains in packages/cli/src; both arms gate on the public optional displayName and delegate localization to getExtensionDisplayName, with the why-comment on the active arm. lists a duplicate Skill name within one inactive extension only once asserts no extensionDisplayName key; lists active and inactive extension Skills without a runtime Config asserts extensionDisplayName: 'inactive display' on the inactive entry.
  • R3-4 (rc:3955145215, Suggestion) — doc guarantee vs per-artifact loader faults. Fixed at 8d5907ed26: the doc distinguishes absent roots, propagated root/store errors and loader-owned artifact handling; keeps the shared loader behavior for malformed extension manifests and reports propagated errors for a dangling extension entry pin both directions.

Resolved by this round's commit

  • R4-3 (rc:3958206500, Suggestion) — "this load is not a read". Took the finding's own in-footprint option: the design doc now declares the reconcile contract — with a present extensions root, the status read initializes a missing or drifted store in place under the store's exclusive lock (rewriting extension-store/state.json, creating the state.previous.json rollback copy, rewriting the legacy extension-enablement.json projection), and writes nothing once reconciled. The non-reconciling read alternative needs a core ExtensionStore read-only entry point and is out of this PR's footprint; the requested extension-store-absent test pins that unchosen alternative (it is red by construction under the declared contract), so no test is added — the reconcile path itself is exercised by every extension-backed case in the suite.
  • R4-4 (rc:3958206508, Suggestion) — missing "Known later-stage items" record. Restored as a compact 4-bullet list carrying exactly the standing items (sibling route locale resolution without skipLoadEnvironment; duplicated inactive-append/sort assembly; config-catalog invalidation + refreshCacheIfSourcesChanged revalidation assigned to stage 4; hand-rolled enabled mirror of Config.isSkillEnabled), excluding the withdrawn machinery (epoch/coalescing, partial-success, --enable-extension). Each bullet was re-verified against HEAD before being written.

Declined with evidence (thread left open)

  • R4-1 (rc:3958206471, Critical) — per-extension failure domain / inner degrade. The degrade is the partial-success machinery the author's scope correction (ic:5578271211) explicitly rejected and withdrew: enumeration failures must report uninitialized/error, and does not cache a failed store read as an initialized empty catalog plus reports propagated errors for a dangling extension entry pin exactly that contract, so re-pointing them at initialized: true is declined. The finding's own alternative is implemented instead: the doc's error paragraph now states the all-or-nothing blast radius explicitly (one bad extension artifact fails the entire catalog — project, user and bundled Skills included — until repaired) and records that per-level degradation is a stage-4 decision. The discovery-disabled population keeps its inactive management entries (matching the child, per the author's R1-6 disposition) under the same contract.

Escalated — decision not this bot's to make (threads left open)

  • R3-1 (rc:3955145189 + rc:3958206479, Critical) — config-catalog providers are not invalidated by extension mutations. The mechanism is re-confirmed at this head (no invalidateSkillsConfigStatus call site in either extension route file), but the author has explicitly staged all cache/invalidation work to PR 4 of tracking(daemon): Decouple Skill management from child and refresh explicit sessions in PRs under 1000 lines #11274 and requested @wenshao's ruling on the stage boundary. This round neither implements the wiring nor dismisses the finding; the staleness is now recorded in-repo in the design doc's later-stage items so the ruling and the stage-4 implementer see it.
  • R4-2 (rc:3958206488, Critical) — invalidate swallowed by an in-flight cold build. The finding's new evidence (the cached extension snapshot makes the swallow freeze an inventory where the base provider froze nothing) genuinely narrows the author's "present at the base" reasoning, and the ~6-line epoch guard plus its already-written regression test is small — but that guard is exactly the machinery class the author's scope correction withdrew and assigned to stage 4. Restoring it here vs holding the stage line is the maintainer's call; the thread carries the narrowed question.

Notes

  • Standing already-tracked items (R2-5 locale normalization, R1-7 cold-build load/revalidation, MCP data-dir creation, store lock wait) remain on their existing threads/deferral records; nothing new was added for them.
  • Reviewer-disclosed gaps (no Windows/macOS CI witness at this commit; mutation-probe harness unvalidated) need no code action.

Verification

  • npm run build — passed (required once to build workspace dist/ prerequisites for the CLI test guard)
  • npm run typecheck — passed
  • npm run lint — passed
  • npx prettier --check docs/design/daemon-extension-skill-catalog.md — passed
  • cd packages/cli && npx vitest run src/serve/workspace-skills-status.test.ts — 38/38 passed
  • No code changed, so no new test witnesses are required; the doc edits quote only behavior pinned by existing tests cited above.
  • Pre-commit hook (lint-staged) ran on commit and passed.
中文说明

Autofix 本轮处理 — PR #11281(address-review)

提交:cf66404d03 —— docs(daemon): record skills-catalog read contract and later-stage items (#11281)(仅文档,+29/-5;PR 总量仍约 735/1000 行)。

本轮刻意只实现最小阻塞子集(已注意到预算警告):两条在 PR 范围内的文档类 Suggestion,以及第 4 轮 Critical 自己给出的文档化备选方案。三条存在分歧的 Critical 均以证据升级或婉拒,没有被悄悄丢弃。

反馈处置

已由先前提交解决、并在当前 head 重新核实

  • R3-2(rc:3955145195,Suggestion) —— 根目录缺失时的 store 创建。已在 8d5907ed26 修复:workspace-skills-status.tslstat 探测结果门控 manager 构造(if (entry)),且 does not create an extension store when no extensions directory exists 用例钉住 refreshCache 未被调用、extensions/extension-store/ 均未被创建。本轮套件运行中为绿。
  • R3-3(rc:3955145205,Suggestion) —— 读取 _rawLocalizable 私有字段。已在 8d5907ed26 修复:packages/cli/src 中已无 _rawLocalizable 引用;两个分支都改为检查公开的可选 displayName 字段并委托 getExtensionDisplayName 做本地化,active 分支附有 why 注释。lists a duplicate Skill name within one inactive extension only once 断言不含 extensionDisplayName 键;lists active and inactive extension Skills without a runtime Config 在 inactive 条目上断言 extensionDisplayName: 'inactive display'
  • R3-4(rc:3955145215,Suggestion) —— 文档保证与逐条 loader 故障不符。已在 8d5907ed26 修复:文档区分了根目录缺失、被传播的根/store 错误与 loader 自有的单条目处理;keeps the shared loader behavior for malformed extension manifestsreports propagated errors for a dangling extension entry 两个回归用例钉住了两个方向。

由本轮提交解决

  • R4-3(rc:3958206500,Suggestion) —— 「这次加载不是一次读取」。采用该发现自己给出的、在 PR 范围内的选项:设计文档现在声明了协调契约 —— extensions 根目录存在时,状态读取会在 store 独占锁下就地初始化缺失或漂移的 store(重写 extension-store/state.json、创建 state.previous.json 回滚副本、重写旧版 extension-enablement.json 投影),协调完成后后续读取不再写入。非协调读取的替代方案需要 core 的 ExtensionStore 只读入口,超出本 PR 范围;发现所要求的「extension-store 不存在」断言钉的是那个未被选择的替代方案(在被声明的契约下它必然为红),因此不新增该测试 —— 协调路径本身已被套件中每个带 extension 的用例覆盖。
  • R4-4(rc:3958206508,Suggestion) —— 缺失「Known later-stage items」记录。已恢复为紧凑的 4 条清单,只收录仍然成立的条目(兄弟路由不加 skipLoadEnvironment 的 locale 解析;与 child 重复的 inactive 追加/排序装配;指派给第 4 阶段的 config 目录失效接线与 refreshCacheIfSourcesChanged 重新校验;手写复刻 Config.isSkillEnabledenabled 判断),并排除了描述已撤回机制的条目(epoch/在途合并、partial-success、不存在的 --enable-extension)。每条写入前均已对照 HEAD 重新核实。

附证据婉拒(线程保持打开)

  • R4-1(rc:3958206471,Critical) —— 单条目失败域 / 内层降级。该降级正是作者范围澄清(ic:5578271211)明确拒绝并撤回的 partial-success 机制:枚举失败必须报告为 uninitialized/error,而 does not cache a failed store read as an initialized empty catalogreports propagated errors for a dangling extension entry 钉住的正是这一契约,因此把这两个用例改指 initialized: true 的要求被婉拒。改为实现该发现自己给出的备选方案:文档的错误段落现在明确写出 all-or-nothing 的影响面(一个损坏的 extension 制品会让整份目录失败 —— 包括 project、user、bundled 三层 —— 直到制品被修复),并记录「按层级降级」属于第 4 阶段的决策。关闭了发现层级的群体仍保留 inactive 管理条目(按作者对 R1-6 的处置与 child 一致),并适用同一契约。

升级处理 —— 不属于本 bot 可自行裁定(线程保持打开)

  • R3-1(rc:3955145189 + rc:3958206479,Critical) —— extension 变更不会失效 config 目录 provider。机制已在当前 head 重新确认(两个 extension 路由文件均无 invalidateSkillsConfigStatus 调用点),但作者已明确把全部缓存/失效工作划入 tracking(daemon): Decouple Skill management from child and refresh explicit sessions in PRs under 1000 lines #11274 的 PR 4,并已请 @wenshao 裁定阶段边界。本轮既不实现该接线,也不驳回该发现;这份陈旧性现已通过设计文档的 later-stage 清单记录在仓库内,供裁定者与第 4 阶段实现者查阅。
  • R4-2(rc:3958206488,Critical) —— 冷构造在途时送达的 invalidate 被吞。该发现的新证据(缓存的 extension 快照让被吞的 invalidate 冻结一份清单,而 base 的 provider 无可冻结之物)确实收窄了作者「base 上同样存在」的论证,且约 6 行的 epoch 保护加上已写好的回归用例体量很小 —— 但该保护正是作者范围澄清所撤回并划入第 4 阶段的机制类别。在本 PR 恢复它还是坚守阶段边界,应由维护者裁定;线程中已留下收窄后的问题。

备注

  • 已在册的常驻条目(R2-5 locale 归一化、R1-7 冷构造加载/重新校验、MCP 数据目录创建、store 锁等待)仍留在各自已有线程/延后记录中,本轮未为其新增内容。
  • 评审披露的缺口(本次提交缺 Windows/macOS CI 见证;变异探针 harness 未验证)无需代码动作。

验证

  • npm run build —— 通过(CLI 测试前置守卫要求先构建各 workspace 的 dist/)
  • npm run typecheck —— 通过
  • npm run lint —— 通过
  • npx prettier --check docs/design/daemon-extension-skill-catalog.md —— 通过
  • cd packages/cli && npx vitest run src/serve/workspace-skills-status.test.ts —— 38/38 通过
  • 本轮无代码变更,因此不需要新的测试见证;文档改动只陈述由上文所列既有测试钉住的行为。
  • 提交时 pre-commit 钩子(lint-staged)已运行并通过。

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

🧵 Resolved all 5 selected review thread(s). · 已关闭全部选中的 5 条评审线程。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 kimi-k3 · CLI 0.23.1

@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.

Partially reviewed — gaps disclosed.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-9 inactive-append and sort assembly duplicated from the child producer, dedupe rule already differing — already reported (comment 3950188533), author deferred and recorded in the design doc
  • R1-7 full locked extension-store reload on every cache miss, and invalidate discarding the cached manager — already reported (comment 3950188523)
  • N-1 hand-derived enabled judgment instead of Config.isSkillEnabled — already reported (@ytahdn review 5130485638) and recorded in the design doc's later-stage items
  • R3-3 active-path display-name guard unasserted — already reported (comment 3955145205), its MUTANT A already witnessed there

Not reviewed: test-efficacy — the mutation/hunk probe could not validate its own control this round (harnessValidated: null; the one whole-file revert probe came back inconclusive with reason 'no-output', 6 hunks were skipped for the cap and 1 mutant for no green baseline), so this PR's automated mutation and per-hunk coverage is unmeasured; four mutants were instead run by hand during verification.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI at this commit and the packages/cli suite ran on Linux only, so the changed package has no Windows or macOS witness (the new dangling-symlink and chmod cases are skipIf win32 and have no Windows witness either).

Not explored to full depth (tool budget reached): "agent 2": I did not execute a runtime check that a workspace's effectiveEnv QWEN_HOME actually reaches the spawned ACP child's Storage (the basis for Finding A's fa….

5 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).

Deferred under the convergence posture (round 5, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:

  • docs/design/daemon-extension-skill-catalog.md:42 — [review] R5-1: This sentence is the recorded answer to the "this load is not a read" thread, and its steady-state guarantee does not hold: prepareDirectories() re-asserts the store direct…
  • docs/design/daemon-extension-skill-catalog.md:62 — [review] R4-4: (fix-induced) The later-stage-items list restored to answer this finding states that the duplicated assembly keeps the child's level:extensionName:name dedupe key. The code…
  • packages/cli/src/serve/workspace-skills-status.ts:254 — [review] R5-2: No test pins that the appended inactive-extension entries participate in this sort — only the SkillManager -produced prefix is order-asserted, so moving the sort above …
  • packages/cli/src/serve/workspace-skills-status.ts:155 — [review] R5-3: The throw error half of this guard has no test that discriminates it — collapsing the catch to .catch(() => undefined) ships green, and the guard is masked by its ne…
  • packages/cli/src/serve/workspace-skills-status.ts:223 — [review] R5-4: The skill-name-side normalization in this hand-written enabled judgment is unpinned — every fixture skill name is already lowercase, so dropping .trim().toLowerCase()…
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):test-efficacy — the mutation/hunk probe could not validate its own control this round (harnessValidated: null; the one whole-file revert probe came back inconclusive with reason 'no-output', 6 hunks were skipped for the cap and 1 mutant for no green baseline), so this PR's automated mutation and per-hunk coverage is unmeasured; four mutants were instead run by hand during verification.

未审查(原文为英文):build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI at this commit and the packages/cli suite ran on Linux only, so the changed package has no Windows or macOS witness (the new dangling-symlink and chmod cases are skipIf win32 and have no Windows witness either).

未探索到全部深度(达到工具调用预算):"agent 2"I did not execute a runtime check that a workspace's effectiveEnv QWEN_HOME actually reaches the spawned ACP child's Storage (the basis for Finding A's fa…

5 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效;CLI 已将其移入下方延后清单(下限强制执行)。

收敛姿态下延后(第 5 轮,非阻断)——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [Lint & Static (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [Lint & Static (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@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.

Partially reviewed — gaps disclosed.

3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • RA3-1 daemon-local locale reads the shared process QWEN_CODE_LANG, so one workspace's .env can override another workspace's display language — same root as already-reported R1-5 in the opposite direction (comment 3950188503)
  • S-3 per-workspace rebuild and retention of a workspace-global extension inventory — already reported as R1-7 (comment 3950188523)
  • S-7 active-path display-name guard unasserted — already reported as R3-3 (comment 3955145205)

2 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R4-1, R3-1) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.

Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI at this commit and the packages/cli suite ran on Linux only, so the changed package has no macOS or Windows witness (the new dangling-symlink and chmod cases are skipIf win32 and have no Windows witness either).

Not reviewed: test-efficacy — the mutation/hunk probe could not validate its own control this round (harnessValidated: null; the one whole-file revert probe came back inconclusive with reason 'no-output' because the probe tree lacks the gitignored packages/cli/src/generated/git-commit.ts, 6 hunks were skipped for the cap and 1 mutant for no green baseline), so this PR's automated mutation and per-hunk coverage is unmeasured; four mutants were instead run by hand during verification.

Not explored to full depth (tool budget reached): "agent 6c": I did not run packages/cli 's vitest for workspace-skills-status.test.ts , so the green-ness of the new tests (in particular whether refreshCache would inde…; "agent 6c": I did not confirm that the ACP child spawn actually applies runtimeEffectiveEnv (including QWEN_HOME ) as its process env — I verified it is threaded as env…; "agent reverse-audit (round 3)": an executed reproduction of finding 1 (plant QWEN_CODE_LANG through one workspace's .env via resolveExtensionLocale , then read a second workspace's config…; "agent reverse-audit (round 6)": did not read ExtensionStore.ensureInitializedUnlocked below extension-store.ts:485 (the no-existing-snapshot, orphan and declarationOnly branches), so the d…; "agent reverse-audit (round 6)": did not execute a live A/B differential between this provider's wire output and the ACP child's buildWorkspaceSkillsStatus on a shared fixture; the parity cla…, and 1 more.

Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/serve/workspace-skills-status.ts:150 — [probe] D6-1 Both read-only guarantees are hand-rolled at one call site, so GET /extensions answers 200 {extensions: []} while GET /workspace/config/skills answers initialized:false + …
  • packages/cli/src/serve/workspace-skills-status.ts:220 — [probe] D6-2 The hand-written enabled judgment duplicates buildExtensionSkillStates, a Config-free copy of the same rule in this package; a 13-case sweep found 0 divergences today and …

Convergence: round 6 posted 4 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (0 new). Findings keep coming back to the same files: packages/cli/src/serve/workspace-skills-status.ts (findings in rounds 3, 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (4 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):

standing Critical attack surface attacker-dependency blast radius
(each standing Critical)

Advisory only — it does not block this review.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 3 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

本轮评审重新推导出的 2 条候选发现与本 PR 已携带的条目匹配,已在验证前搁置(R4-1, R3-1)——被匹配的已发布条目照常在上一轮状态区裁定,被匹配的延后条目仍保留在延后清单记录中。

未审查(原文为英文):build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI at this commit and the packages/cli suite ran on Linux only, so the changed package has no macOS or Windows witness (the new dangling-symlink and chmod cases are skipIf win32 and have no Windows witness either).

未审查(原文为英文):test-efficacy — the mutation/hunk probe could not validate its own control this round (harnessValidated: null; the one whole-file revert probe came back inconclusive with reason 'no-output' because the probe tree lacks the gitignored packages/cli/src/generated/git-commit.ts, 6 hunks were skipped for the cap and 1 mutant for no green baseline), so this PR's automated mutation and per-hunk coverage is unmeasured; four mutants were instead run by hand during verification.

未探索到全部深度(达到工具调用预算):"agent 6c"I did not run packages/cli 's vitest for workspace-skills-status.test.ts , so the green-ness of the new tests (in particular whether refreshCache would inde…"agent 6c"I did not confirm that the ACP child spawn actually applies runtimeEffectiveEnv (including QWEN_HOME ) as its process env — I verified it is threaded as env…"agent reverse-audit (round 3)"an executed reproduction of finding 1 (plant QWEN_CODE_LANG through one workspace's .env via resolveExtensionLocale , then read a second workspace's config…"agent reverse-audit (round 6)"did not read ExtensionStore.ensureInitializedUnlocked below extension-store.ts:485 (the no-existing-snapshot, orphan and declarationOnly branches), so the d…"agent reverse-audit (round 6)"did not execute a live A/B differential between this provider's wire output and the ACP child's buildWorkspaceSkillsStatus on a shared fixture; the parity cla…,另有 1 条。

收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 6 轮发布了 4 条行内评论,其中 1 条是首次提出;上一轮发布了 3 条(其中 0 条首次提出)。发现反复回到同一批文件:packages/cli/src/serve/workspace-skills-status.ts(第 3、4 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 4 条 Critical),首次发现的速率没有下降(本轮 1,上一轮 0),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。

— qwen3.8-max via Qwen Code /review (v0.23.1)

Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
Comment thread packages/cli/src/serve/workspace-skills-status.ts
Comment thread packages/cli/src/serve/workspace-skills-status.ts Outdated
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run.

中文说明

🔀 已更新 base:红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。

@callmeYe callmeYe removed the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Sep 9, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply autofix/takeover (or comment @qwen-code /takeover) to re-engage.

中文说明

👋 已释放:autofix 循环不再介入此 PR(在飞的一轮如有,将完成其有界工作)。重新打上 autofix/takeover 标签(或评论 @qwen-code /takeover)即可再次接管。

@callmeYe

callmeYe commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review closure at 95859798cf9ff9677f1d2c0dd3c9beca30f1f67d:

R6-1 is fixed, not merely deferred. The provider obtains the existing consistent ExtensionStore snapshot and resolves each manifest's Skill default from the already loaded extension object. Workspace overrides use the same snapshot and existing store resolver. It no longer reselects an owner through the ambiguous by-ID manager method. There is no catch-to-undefined/default-enable fallback. The regression uses two manifest names sharing one repository ID, checks repeated reads, preserves the second manifest's disabled default, and checks both workspace-disabled and workspace-enabled overrides. The core identity model and extension state API remain outside this PR.

The remaining recurring findings are explicitly disposed within the author's approved staging. R3-1 (config-catalog invalidation), R1-4/R4-2 (in-flight invalidation epochs) remain tracked stage-4 work under #11274. Their observable stale-state limitation is acknowledged; no claim is made that this revision implements them. The author explicitly accepted the stage-2 improvement and prohibited adding these cache/concurrency changes. Reposting the same staged requirement does not create a new in-scope implementation obligation.

R1-1/R4-1 asks for extension-only partial-success degradation. That contradicts the established stage-2 error contract. Propagated directory/store failures remain explicit uninitialized/error responses, with their all-or-nothing impact documented and tested. Individual artifact skipping still belongs to the existing shared loader. This requested contract change is rejected, not silently implemented or described as fixed.

All repeated threads for these roots are closed with those dispositions. The known limitations remain in the design and tracking issue. Additional Suggestion-level extraction, mutation-test coverage and sibling-route cleanup are follow-up work after six review rounds, not further scope in this PR. Two inaccurate documentation statements about write-free rebuilds and the literal dedupe key were corrected while updating the required bilingual design.

Earlier R6 diagnosis correctly rejected the unsafe fallback; the bounded owner/snapshot fix above supersedes the earlier decision to leave the provider failure unresolved. Required repository approval and new-head CI remain required. Clearing superseded automated review metadata is not self-approval or a claim that deferred stage-4 work is implemented.

Validation: full build, typecheck and bundle passed; 177 focused provider/mapping/facade tests passed. The collision regression was red before the fix. Six real HTTP identity/default/override scenarios plus the previous twelve daemon scenarios passed (18 total). No child, sessions or MCP/hooks were required. Two clean diff audits, local review, lint/format and the bilingual design checks passed. Total PR diff including both design documents: 866 additions + 31 deletions = 897 lines.

@callmeYe
callmeYe dismissed stale reviews from qwen-code-ci-bot, qwen-code-ci-bot, and qwen-code-ci-bot September 9, 2026 06:03

Superseded after active resolution requested by the author. At 9585979, R6-1 is fixed by owner-object/default plus consistent-store-override resolution (no catch-to-enabled); 177 focused tests and 18 real daemon scenarios passed. Repeated R3-1/R4-2 are author-accepted stage-4 deferrals, and R4-1 partial-success degradation is rejected by the documented stage-2 error contract. Full dispositions: #11281 (comment) . All 11 corresponding threads are resolved; no current human blocking review or unhandled thread remains. This clears stale automated review metadata, not required current-head CI or maintainer approval.

@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.

Partially reviewed — gaps disclosed.

6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

1 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R4-1) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent 6a": whether updateLegacyProjectionRemainder ( extension-store.ts:606-609 ) can oscillate between the daemon's user-only identity set and a CLI/child's superset, w…; "agent 6a": whether any workspace runtime's effectiveEnv can actually carry a QWEN_HOME (the premise of finding 4) — I confirmed getRuntimeEffectiveEnv supports a non…; "agent reverse-audit (round 1)": whether any production createServeApp caller enables workspaceTrustHotReloadAvailable without an injected registry or deps.primaryWorkspaceTrusted — the c….

Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/serve/workspace-skills-status.ts:161 — [review] fail-closed root probe contradicts the sibling /workspace/extensions route over the same directory
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 6 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

本轮评审重新推导出的 1 条候选发现与本 PR 已携带的条目匹配,已在验证前搁置(R4-1)——被匹配的已发布条目照常在上一轮状态区裁定,被匹配的延后条目仍保留在延后清单记录中。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent 6a"whether updateLegacyProjectionRemainder ( extension-store.ts:606-609 ) can oscillate between the daemon's user-only identity set and a CLI/child's superset, w…"agent 6a"whether any workspace runtime's effectiveEnv can actually carry a QWEN_HOME (the premise of finding 4) — I confirmed getRuntimeEffectiveEnv supports a non…"agent reverse-audit (round 1)"whether any production createServeApp caller enables workspaceTrustHotReloadAvailable without an injected registry or deps.primaryWorkspaceTrusted — the c…

收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.1)

Comment thread packages/cli/src/serve/workspace-skills-status.ts
Comment thread packages/cli/src/serve/workspace-skills-status.ts
@callmeYe
callmeYe dismissed qwen-code-ci-bot’s stale review September 9, 2026 10:15

Duplicate automated blockers at unchanged head 9585979. R4-1 retains the explicitly documented all-or-nothing error contract; R4-2 epoch/coalescing remains an author-accepted stage-4 deferral. The round-7 witnesses confirm already acknowledged consequences and do not change those dispositions. Both repeated threads now carry exact-head replies and are resolved; no human blocking review is present. R6 was fixed in this head. See #11281 (comment) . Clearing duplicate metadata does not claim deferred work is implemented or waive required approval/CI.

@wenshao

wenshao commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Independent runtime verification — real qwen serve daemons on Linux

I built a local end-to-end harness for this PR and drove real HTTP against real qwen serve daemons — not the unit-test seam. This covers the row your "Tested on" table marks as ⚠️ not tested locally (Linux).

Setup

  • Head 95859798cf9ff9677f1d2c0dd3c9beca30f1f67d, merge-base 88b0afaeb1bb, Linux 6.12 / Node 22.22.2, full npm run build.
  • A/B by swapping only packages/cli/dist/src/serve/workspace-skills-status.js between the merge-base and PR-head builds — identical fixtures, identical binary otherwise.
  • Isolated HOME + QWEN_HOME per scenario; qwen serve --port 0 --no-web --workspace <ws> with the default --http-bridge (no VITEST_WORKER_ID or any other test hook). Dozens of daemon launches.
  • Fixtures: alpha (active, skillStates: {alpha-off: false}, MCP-command sentinel), beta (active, displayName as an {en, zh, zh-CN} locale map), gamma + epsilon (deactivated via extension-enablement.json), delta (active, skill name colliding with a project skill), plus a project SKILL.md.

Verdict: the feature does what the description says, and it matches the ACP child exactly. Three behaviours on the failure and contention side are new relative to the merge-base and I think deserve an explicit maintainer decision before merge. None of them is a defect in the mapping logic itself.


1. The headline claim reproduces on a genuinely cold daemon

sessions.active=0, channel.live=false, no prompt ever sent. Merge-base returns 15 skills with zero extension rows; this PR returns 20, with active skills, manifest-default-disabled skills, and inactive_extension management entries carrying their own extension identity.

Project precedence holds: the project shared-skill wins over delta's active same-name extension skill (which is absent), while epsilon's inactive shared-skill is still listed separately with its own identity. The MCP command sentinel in alpha's manifest never executed.

2. The daemon-local catalog is byte-identical to the authoritative ACP child

Two daemons on identical fixtures. A never creates a session (this PR's new path). B creates a session so a real qwen --acp child answers /workspace/skills. Compared field by field:

Same set, same status / disabledReason / extensionDisplayName / installedPath / modelInvocable / userInvocable / argumentHint, and the same list order. This is the strongest evidence for the "mirror the child's producer" design choice, and it holds for skills.disabledLevels: ["extension"] too (both sides keep the inactive-extension rows).

3. Settings, trust, safe mode and locale

One fresh cold daemon per row:

  • Manifest skillStates defaults, skills.enabled, skills.disabled (→ hard), skills.defaultDisabled all resolve correctly.
  • skills.enabled: ["gamma-skill"] correctly does not activate an inactive extension's skill.
  • Persisted skillWorkspaceOverrides in extension-store/state.json are read back on a brand-new cold daemon and flip both directions.
  • Untrusted workspace: no ExtensionManager, no extension-store created at all.
  • Locale resolves per language and re-projects from the cached raw manifest without restarting the daemon (verified on /workspace/config/skills, which has no snapshot latch).

Two more checks worth recording:

  • The EACCES arm your suite skips under uid 0 (workspace-skills-status.test.ts:874) — I ran it for real, daemon running as nobody with extensions mode 000. It fails closed with an explicit EACCES error exactly as designed.
  • Cost: the first cold read costs +40 ms at 30 installed extensions and +155 ms at 150; steady state is ~2 ms on both arms. Not a concern.
  • Your focused suites pass here: 38 provider + 132 facade + 6 mapping = 176 tests green.

Findings for a maintainer decision

F1 — An unusable ~/.qwen/extensions now empties the whole pre-session catalog

Merge-base: initialized: true, 14 skills, /review present. This PR: initialized: false, skills: [], /review gone. Reproduced two ways — a stray file at $QWEN_HOME/extensions (ENOTDIR) and an unreadable directory under a non-root daemon user (EACCES).

I know this is deliberate — workspace-skills-status.test.ts:621 pins it, and you explicitly rejected the extension-only degradation request (R1-1/R4-1). The unit test can't show the blast radius though: at the HTTP level, one broken extensions path removes every bundled and project skill from the Web Shell and from slash-command autocomplete, on a route that previously never touched that directory. That is the trade a maintainer should be agreeing to, not just the initialized:false shape.

F2 — A contended extension-store lock stalls this read-only route for ~27 s, then returns an empty catalog

GET /workspace/skills is a read-only status route. With the store lock held by another process — which is exactly what qwen extensions install/update/uninstall does — it now blocks for 27.0 s on the ExtensionStore.withLock retry ladder (extension-store.ts:1483, retries: 60), and returns HTTP 200 with initialized:false, skills:[]. Three sequential reads measured 27.0 s / 27.0 s / 53.7 s (the third queued behind another store attempt); three parallel reads are coalesced by the facade and each take ~27.1 s. The merge-base answers instantly with the full catalog, and recovery is immediate once the lock frees.

I could not find any test covering the busy path — the triage bot raised the question, but nothing pins the behaviour. This is the finding I'd most want addressed: a retries: 0 / bounded-wait read for this route, or degrading only the extension level, would remove a half-minute stall in the Web Shell that a user cannot explain.

F3 — A read-only GET now initialises the extension store on disk

One cold GET /workspace/skills creates $QWEN_HOME/extension-store/ with state.json, lock, rollback/, staging/, transactions/. Nothing comparable happens on the merge-base. To be fair to the PR: it is a one-time initialisation (repeat reads leave state.json byte-identical and don't bump generation), and an absent extensions root correctly creates nothing. Still, "a GET writes to ~/.qwen" is a contract change worth stating in the description rather than leaving implicit.

F4 — qwen serve --safe-mode does not suppress extension skills

The Reviewer Test Plan says "Verify safe mode … do not load extension runtime context". That holds for QWEN_CODE_SAFE_MODE=1, which the provider reads through isSafeModeEnv(). It does not hold for the documented CLI flag: under qwen serve --safe-mode the full extension catalog is returned (row 9 of the matrix above).

This is pre-existing — the flag isn't plumbed into the daemon-local provider, and the ACP child ignores it too, so parity is intact and this is not a divergence. But safe mode is documented as "Disable all customizations (context files, hooks, extensions, skills, MCP servers) for troubleshooting", and this PR is what makes extension data appear under that flag. Either thread the resolved flag into WorkspaceSkillsStatusProviderOptions, or soften the claim in the test plan.

F5 (minor) — skills.disabledLevels: ["extension"] still pays for the extension level

The gate at workspace-skills-status.ts:152 is workspaceTrusted && !safeMode; it doesn't consult disabledLevels. So a user who disabled the extension level still constructs an ExtensionManager, takes the store lock and reads every manifest — and SkillManager then discards the result. Harmless on its own; combined with F2 it's an avoidable 27 s stall for someone who opted out of the level entirely. (The inactive-extension rows that survive disabledLevels match the child exactly, so that part is consistent — see §2.)


Checked and not a regression

  • Snapshot freshness. /workspace/skills freezes after the second read until an explicit invalidation, so an out-of-band qwen extensions install isn't picked up and a deleted extension keeps being advertised with a dead installedPath. I measured this on both arms with project skills and it is identical — pre-existing facade behaviour (workspace-service/index.ts lastWorkspaceSkillsStatus), not something this PR introduces. The daemon's own routes do the right thing: POST /workspace/extensions/install and POST /workspace/extensions/:name/disable {"scope":"workspace"} both invalidate and the catalog updates correctly.
  • Same-name extension directories. Two install dirs sharing one manifest name with conflicting skillStates: only one loads, and its default is applied to its own skill. No wrong-owner state observed, so the R6 fix holds at the name level too.
  • Case-variant skill names in one inactive extension produce two rows — the child's producer does the same (its dedupe key uses the raw name), so this is parity, not a defect.
  • Malformed manifests are skipped by the shared loader without failing the response.

Summary

The mapping and gating work is genuinely good, and the exact-match against the ACP child is a stronger result than the description claims. What I'd want settled before merge, in priority order: F2 (a 27 s stall on a read-only route, with no test pinning it), then F1 and F3 as explicit contract decisions, then F4 as either a small fix or a wording change. F5 is a cheap one-line guard that also mitigates F2 for opted-out users.

Repro
# 1. worktree at the PR head, full build
git worktree add <wt> <pr-head> && cd <wt> && npm run build

# 2. isolated fixture tree
#    $QWEN_HOME/extensions/<name>/qwen-extension.json         (+ skillStates, displayName locale map)
#    $QWEN_HOME/extensions/<name>/skills/<skill>/SKILL.md
#    $QWEN_HOME/extensions/extension-enablement.json          {"gamma":{"overrides":["!*"]}}
#    $WS/.qwen/skills/<skill>/SKILL.md                        (project precedence)

# 3. cold daemon, no session
env -i PATH=... HOME=$ROOT/home QWEN_HOME=$ROOT/qwenhome \
  node packages/cli/dist/index.js serve --port 0 --hostname 127.0.0.1 --no-web --workspace $WS
curl -s http://127.0.0.1:$PORT/workspace/skills

# 4. A/B: swap only the compiled provider
git show <merge-base>:packages/cli/src/serve/workspace-skills-status.ts > base.ts
npx esbuild --format=esm --platform=node --target=node22 --outfile=base.js base.ts
cp base.js packages/cli/dist/src/serve/workspace-skills-status.js   # restart, re-query

# 5. F2 — hold the store lock from another process and time the GET
mkdir -p $QWEN_HOME/extension-store/lock.lock
while :; do touch $QWEN_HOME/extension-store/lock.lock; sleep 1; done &
curl -s -o /dev/null -w '%{time_total}\n' http://127.0.0.1:$PORT/workspace/skills
中文版

独立运行时验证 —— Linux 上的真实 qwen serve daemon

我在本地搭建了端到端验证环境,直接对真实 qwen serve daemon 发起真实 HTTP 请求(而非单元测试接缝),补上了你 "Tested on" 表格中标记为 ⚠️ 未本地验证的 Linux 一行。

环境:head 95859798、merge-base 88b0afae、Linux 6.12 / Node 22.22.2、完整 npm run build。A/B 替换 packages/cli/dist/src/serve/workspace-skills-status.js,其余二进制与 fixture 完全一致。每个场景独立 HOME + QWEN_HOMEqwen serve --port 0 --no-web --workspace <ws>,保留默认 --http-bridge未使用 VITEST_WORKER_ID 等测试钩子),共数十次 daemon 启动。fixture:alpha(active,skillStates 默认禁用一个 skill,带 MCP command 哨兵)、beta(active,displayName{en, zh, zh-CN} 语言映射)、gamma + epsilon(通过 extension-enablement.json 停用)、delta(active,skill 名与 project skill 冲突),外加一个 project SKILL.md

结论:功能与描述一致,且与 ACP child 的输出完全一致。 但在失败与锁竞争路径上有三项行为相对 merge-base 是新增的,建议合并前由 maintainer 明确决策。这些都不是映射逻辑本身的缺陷。

已确认可用

  1. 冷 daemon 复现头号主张sessions.active=0channel.live=false、从未发过 prompt。merge-base 返回 15 个 skill、 extension 条目;本 PR 返回 20 个,包含 active skill、manifest 默认禁用的 skill,以及保留自身 extension 身份的 inactive_extension 管理条目。project shared-skill 正确优先于 delta 的同名 extension skill,而 epsilon(inactive)的同名条目仍以独立身份列出。alpha manifest 中的 MCP command 哨兵从未执行。
  2. 与权威 ACP child 完全一致:两个 daemon 使用相同 fixture,A 从不创建会话(本 PR 新路径),B 创建会话让真实 qwen --acp child 应答。逐字段对比结果:集合相同、status/disabledReason/extensionDisplayName/installedPath/modelInvocable/userInvocable/argumentHint 全部相同,连列表顺序都一致。在 skills.disabledLevels: ["extension"] 下同样成立(两侧都保留 inactive-extension 条目)。
  3. 配置矩阵:manifest skillStates 默认值、skills.enabledskills.disabled(→ hard)、skills.defaultDisabled 均正确;skills.enabled: ["gamma-skill"] 正确地不会启用 inactive extension 的 skill;extension-store/state.json 中持久化的 skillWorkspaceOverrides 在全新冷 daemon 上被正确读回并可双向翻转;未信任 workspace 下不构造 ExtensionManager、完全不创建 extension-store;locale 按语言正确解析,并能在不重启 daemon 的情况下从缓存的 raw manifest 重新投影(在无快照锁存的 /workspace/config/skills 上验证)。
  4. 补充两点:你的测试套件在 uid 0 下会跳过 EACCES 用例(workspace-skills-status.test.ts:874),我以 nobody 用户 + extensions 目录 000 权限真实跑通,确认按设计 fail-closed;性能开销为首次冷读取在 30 个 extension 时 +40 ms、150 个时 +155 ms,稳态约 2 ms,可以接受;你的定向测试在本地全绿(38 provider + 132 facade + 6 mapping = 176 项)。

需要 maintainer 决策的问题

F1 —— ~/.qwen/extensions 不可用时,整个 pre-session 目录被清空。 merge-base:initialized: true、14 个 skill、/review 存在;本 PR:initialized: falseskills: []/review 消失。两种方式复现:$QWEN_HOME/extensions 是普通文件(ENOTDIR),以及非 root daemon 用户下目录不可读(EACCES)。我知道这是有意为之(workspace-skills-status.test.ts:621 已固化,你也明确拒绝了 R1-1/R4-1 的分级降级请求)。但单元测试无法体现影响范围:在 HTTP 层,一个损坏的 extensions 路径会让 Web Shell 和斜杠命令补全失去全部 bundled 和 project skill,而这个路由此前根本不接触该目录。这才是 maintainer 需要认可的代价,而不仅仅是 initialized:false 这个形状。

F2 —— extension store 锁竞争会让这个只读路由阻塞约 27 秒,然后返回空目录。 GET /workspace/skills 是只读状态路由。当锁被其他进程持有(qwen extensions install/update/uninstall 正是如此)时,它会在 ExtensionStore.withLock 的重试阶梯(extension-store.ts:1483retries: 60)上阻塞 27.0 秒,返回 HTTP 200 + initialized:false, skills:[];三次串行读取实测为 27.0 秒 / 27.0 秒 / 53.7 秒(第三次排在另一次 store 尝试之后);三次并行读取被 facade 合并,各约 27.1 秒。merge-base 则立即返回完整目录,锁释放后立即恢复。我没有找到任何覆盖 busy 路径的测试 —— triage bot 提过这个问题,但没有任何测试固化行为。这是我最希望被处理的一项:为该路由改用 retries: 0 / 有界等待,或仅降级 extension 层级,可以消除一次用户无法理解的半分钟卡顿。

F3 —— 只读 GET 现在会在磁盘上初始化 extension store。 一次冷 GET /workspace/skills 会创建 $QWEN_HOME/extension-store/,包含 state.jsonlockrollback/staging/transactions/;merge-base 完全不会。公平地说:这只是一次性初始化(后续读取 state.json 字节不变,generation 不递增),且 extensions 根目录不存在时确实不创建任何东西。但"GET 会写 ~/.qwen"是值得在描述中写明的契约变更。

F4 —— qwen serve --safe-mode 不会屏蔽 extension skill。 Reviewer 验证计划称"确认 safe mode ... 不加载 extension 运行时上下文"。这对 QWEN_CODE_SAFE_MODE=1 成立(provider 通过 isSafeModeEnv() 读取),但对文档化的 CLI flag 不成立qwen serve --safe-mode 下仍返回完整 extension 目录(见矩阵第 9 行)。这是既有问题 —— 该 flag 既未接入 daemon 本地 provider,ACP child 也同样忽略它,因此行为一致,不构成分歧。但 safe mode 的文档写的是"禁用所有自定义(context 文件、hooks、extensions、skills、MCP servers)以便排障",而本 PR 正是让 extension 数据在该 flag 下出现的原因。建议要么把解析后的 flag 接入 WorkspaceSkillsStatusProviderOptions,要么弱化测试计划中的表述。

F5(次要)—— skills.disabledLevels: ["extension"] 仍会付出 extension 层级的代价。 workspace-skills-status.ts:152 的门控是 workspaceTrusted && !safeMode,没有检查 disabledLevels。因此禁用了 extension 层级的用户仍会构造 ExtensionManager、取 store 锁、读取全部 manifest,然后被 SkillManager 丢弃。单独看无害,但叠加 F2 就成了本可避免的 27 秒卡顿。(在 disabledLevels 下仍保留的 inactive-extension 条目与 child 完全一致,这部分是自洽的。)

已核查、不是回归的部分

  • 快照新鲜度/workspace/skills 在第二次读取后会冻结,直到显式失效为止,因此进程外的 qwen extensions install 不会被感知,已删除的 extension 会带着失效的 installedPath 继续出现。我用 project skill 在两个 arm 上测量,行为完全相同 —— 属于既有的 facade 行为(workspace-service/index.tslastWorkspaceSkillsStatus),不是本 PR 引入。daemon 自己的路由则表现正确:POST /workspace/extensions/installPOST /workspace/extensions/:name/disable {"scope":"workspace"} 都会正确失效并刷新目录。
  • 同名 extension 目录:两个安装目录共享同一 manifest nameskillStates 冲突时,只有一个被加载,其默认值应用于自身的 skill,未观察到 owner 错配。R6 的修复在 name 层面同样成立。
  • 同一 inactive extension 中大小写不同的 skill 名会产生两行 —— child 的 producer 行为相同(其去重 key 使用原始 name),属于一致行为而非缺陷。
  • 损坏的 manifest 由共享 loader 跳过,不会导致整体失败。

小结

映射与门控的实现质量很好,与 ACP child 的逐字段一致性甚至强于描述中的主张。合并前建议按优先级处理:F2(只读路由 27 秒卡顿,且无测试固化)、然后 F1F3 作为显式契约决策、F4 可以是小修复或改措辞。F5 是一行防护,同时能为已退出该层级的用户缓解 F2。

@callmeYe
callmeYe requested a review from samuelhsin September 10, 2026 02:25

@ytahdn ytahdn 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.

PR 主旨 / What this PR does

给 daemon 本地(ACP 子进程起来之前)的 /workspace/skills 兜底枚举补上「已安装 extension 的 Skill」这块:之前 getActiveExtensions 被 shim 成 () => [],没有子进程快照时 extension Skill 永远不出现。本 PR 从持久化 extension store 构造一个不绑定运行时 ConfigExtensionManager,把 active extension 喂给 SkillManager(保留 project > user > extension > bundled 优先级),把 inactive extension 的 Skill 以 inactive_extension 禁用管理项追加,按名字排序与子进程对齐;并在每次 provider 返回时从缓存的原始 manifest 用工作区语言重新投影本地化展示名。这是跟踪 issue 11274 的第 2 阶段,显式不引入部分成功语义、provider 并发合并/epoch、facade 缓存改造(这些留给后续阶段)。

Fills the extension-Skill gap in the daemon-local pre-child /workspace/skills fallback (previously getActiveExtensions was shimmed to () => []). It builds an unbound ExtensionManager from the persistent store, feeds active extensions into SkillManager (preserving project > user > extension > bundled precedence), appends inactive-extension Skills as disabled inactive_extension management entries, sorts by name to match the child, and re-projects localized display names from cached raw manifests on every response using the workspace language. This is stage 2 of tracking issue 11274; partial-success semantics, provider coalescing/epoch and facade-cache changes are explicitly out of scope.

审查方式:纯静态,逐 hunk 对照 head tree(95859798),未运行测试/构建。CI 实质 lane(Test / Lint & Static / Serve A/B / Integration no-AK / Desktop Shell)全绿。这是一个已经被充分讨论的 PR:round-7 机器人 ledger 仍挂两条 Critical(R4-1、R4-2),作者在内联线程里逐条rebut/defer 并已请维护者(@wenshao)裁定阶段边界;更早的一条真 Critical(R6-1,by-ID owner 查找)作者称已在本 head 修复。我这次的价值是独立复核这几条,而不是重复它们。
Review method: static only, every hunk cross-checked against the head tree (95859798); no tests/builds run. CI's substantive lanes are green. This PR is already heavily litigated: the round-7 bot ledger still carries two Criticals (R4-1, R4-2), which the author has rebutted/deferred inline and escalated to maintainer judgment (@wenshao); an earlier genuine Critical (R6-1, by-ID owner lookup) is claimed fixed at this head. My contribution here is to independently re-verify these, not repeat them.


🎉 已核实修复 / Verified fixed at this head

R6-1(by-ID owner 查找,早轮 Critical)——确认已修复且实现正确。 extensionSkillStates 现在是 Map<Extension, Map<string, boolean>>,按扩展对象而非 id 建键(:90/:151/:189),键取自 getLoadedExtensions()(:170);查回时 extensionSkillStates.get(extension)(:235)里的 extension 来自 extensions.find(e => e.name === skill.extensionName)(:232),extensions 又是每次调用重取的 getLoadedExtensions()(:227)。我核对了 getLoadedExtensions() 返回 [...this.extensionCache.values()](extensionManager.ts:1554)——是缓存里的同一批对象引用;且 refreshCacheWithSnapshot 只在 if (!cached)(:133-224)内跑,缓存命中时 manager 冻结,所以跨调用的对象身份成立,.get(extension) 不会漏。manifest 默认值按对象读 extension.config.skillStates(:174-178),workspace override 仍按 id 读 store(:181)——因此 id 冲突的复制扩展能保住各自的默认值,只共享 store 的 id 级 override(store 本就按 id 存,属既有设计)。测试 preserves each manifest default and store override when extension ids collide 覆盖此点。

R6-1 (by-ID owner lookup, earlier-round Critical) — confirmed fixed and correct. extensionSkillStates is now keyed by the Extension object, not the id (Map<Extension, Map<string, boolean>>, :90/:151/:189), built from getLoadedExtensions() (:170); the lookup extensionSkillStates.get(extension) (:235) resolves extension via find(e => e.name === skill.extensionName) (:232) over getLoadedExtensions() re-fetched each call (:227). I verified getLoadedExtensions() returns [...this.extensionCache.values()] (extensionManager.ts:1554) — the same cached object references — and refreshCacheWithSnapshot only runs inside if (!cached) (:133-224), so the manager is frozen on cache hits and object identity holds across calls; .get(extension) cannot miss. Per-object defaults come from extension.config.skillStates (:174-178); the store override stays id-keyed (:181), so colliding-ID copies keep their own defaults and share only the id-level store override (the store is id-keyed by design).

我上一轮的 Important(本地枚举没传 locale → 非英文工作区展示名漂移)——确认已解决。 locale 现在从 settings.merged.general?.language 解析(:126-131)并传入 new ExtensionManager({ ..., locale })(:167),展示名在每次返回时用 getExtensionDisplayName(extension, locale) 从缓存的原始 manifest 重新投影(:244 active、:268 inactive),无需重建目录缓存。测试 re-resolves the extension locale when the configured language changesignores a non-string general.language instead of failing the catalog 覆盖。

My prior-round Important (daemon-local manager omitted locale → display-name drift on non-English workspaces) — confirmed resolved. The locale is now resolved from settings.merged.general?.language (:126-131) and passed to new ExtensionManager({ ..., locale }) (:167), with display names re-projected per response from cached raw manifests via getExtensionDisplayName(extension, locale) (:244 active, :268 inactive) without rebuilding the directory cache.


对机器人两条 lingering Critical 的独立裁定 / Independent adjudication of the bot's two lingering Criticals

R4-1「extension 加载没有自己的失败域,一个扩展目录的故障清空整份目录」——我与作者一致:非阻塞

作者在内联里以「文档化的 all-or-nothing 错误契约」rebut:返回成功的部分目录不是本阶段接受的失败策略,root/store 故障向上传播是有意的,partial-success 明确出范围。我独立复核后同意,理由是「一个坏扩展清空全部」这个前提大部分不成立

  • 最常见的单个 manifest 损坏已被共享 loader 隔离——loadExtensionsFromExtensionsDir(extensionManager.ts:1614-1623)调用 loadExtension不传 throwOnError,其 catch(:1782-1783)if (options.throwOnError) throw e(默认 false,:1629)→ 坏 manifest 返回 null 被跳过,目录存活。测试 keeps the shared loader behavior for malformed extension manifests 正是断言这点。
  • 真正会致命并清空全表的只剩目录/store 的 I/O 故障fs.lstat(:154,ENOENT→undefined,其余 rethrow)、被丢弃返回值的 readdir 探针(:161)、loadExtension 里位于 try 之外fs.statSync(extensionManager.ts:1632)、以及 readConsistent store 读。而这些恰是本 PR 文档化的 fail-closed 契约(「extension 目录不可读或 store 读失败 → 明确返回 uninitialized/error」),并有测试 returns an error for an unreadable extension directorydoes not cache a failed store read as an initialized empty catalog 覆盖。
  • 新代码里的逐行 helper 都是全函数、不抛getExtensionDisplayName(i18n.ts:103-110)+ resolveLocalizableString(:38-57),getSkillWorkspaceOverride(extension-store.ts:998-1010,纯 optional-chain 读)。所以不存在「某个 skill 触发抛错清空全表」的路径。

结论:这是一个有意、有文档、有测试的设计选择,不是 Critical。(机器人延后的非阻塞项「:161 readdir fail-closed 探针比 sibling /extensions 路由更严」也属同一有意 fail-closed——sibling routes/workspace-extensions.ts:1841 直接让 refreshCacheWithSnapshot 内部吞掉目录问题。)

The author rebuts R4-1 with the documented all-or-nothing error contract (partial-success is out of scope; root/store failures propagate by intent). I concur, because the premise "one bad extension empties the catalog" is largely false: the common single-manifest fault is already isolated by the shared loader (loadExtension called without throwOnError, its catch at extensionManager.ts:1782-1783 returns null → skipped; test keeps the shared loader behavior for malformed extension manifests). The only catalog-fatal paths left are directory/store I/O faults (lstat :154, the discarded readdir probe :161, loadExtension's fs.statSync outside its try at extensionManager.ts:1632, readConsistent), which are exactly this PR's documented fail-closed contract and are test-covered. The new per-row helpers are total (no throw): getExtensionDisplayName (i18n.ts:103-110) and getSkillWorkspaceOverride (extension-store.ts:998-1010). Not a Critical.

R4-2「cold build 进行中收到的 invalidate() 被吞掉」——竞态属实,但属既有模式 + 作者已明确延后到第 4 阶段;我不作为新阻塞项重提

代码层竞态确实存在:get-miss(:132)→ 一串 await(lstat :154 / readdir :161 / refreshCacheWithSnapshot :169)→ managers.set(:223),而 invalidate = managers.delete(:109)。若 invalidate 落在这个窗口里,它删的是一个尚未写入的键(no-op),随后 in-flight 的 build 把变更前的快照 set 进去 → 陈旧状态越过了这个失效点存活,直到下一次 invalidate 或子进程接管。

但需要如实说明三点,这也是我不把它当新阻塞项的原因:

  1. get/build/set 这套缓存模式是既有的——本 PR 之前缓存的就是一个裸 SkillManager,同样的竞态已在(diff 只是把值类型从 Map<string, SkillManager> 拓宽成 Map<string, WorkspaceSkillManagers>)。本 PR 的「急切缓存 extension 状态」让陈旧 payload 更丰富/更易被观察到,但没有引入竞态机制本身。
  2. 作者已把整套 invalidation/coalescing/epoch 显式延后到 11274 的第 4 阶段(内联 :223、:132 的 closed 说明;相关的 R3-1「extension 变更根本不调用 invalidateSkillsConfigStatus」同样延后到第 4 阶段)。dev-bot 已把这个阶段边界上升给维护者裁定(@wenshao),目前尚无维护者拍板
  3. 有缓解:这是子进程前的 best-effort 兜底(:98-100 注释),有会话后子进程权威重列;invalidate 在每次 skill 安装/删除和 workspace 移除时都会触发(server.ts:962-965、onWorkspaceRemoved :2646),所以下一次变更即自愈。

若维护者不接受这个延后,最省的修法是给每个 cwd 加一个 epoch:build 开始时记录 epoch,managers.set 前比对,只有在 build 期间没有 invalidate(epoch 未变)才写入;invalidatedelete 的同时 epoch++。这样 in-flight 期间被失效的 build 结果会被丢弃而不是覆盖。

The R4-2 race is real in code (get-miss :132 → awaits → managers.set :223, while invalidate = managers.delete :109; an invalidate inside the window is a no-op delete and the in-flight build then installs the pre-mutation snapshot, so staleness survives past the invalidation point). But I do not re-raise it as a fresh blocker, honestly, for three reasons: (1) the get/build/set cache pattern is pre-existing — pre-PR it cached a bare SkillManager with the identical race; this PR only widens the cached value type and makes the stale payload more observable. (2) The author has explicitly deferred the whole invalidation/coalescing/epoch mechanism to stage 4 of 11274 (inline closures at :223/:132; the related R3-1 "extension mutations never call invalidateSkillsConfigStatus" is likewise stage-4), and dev-bot escalated the stage boundary to maintainer judgment (@wenshao) — not yet ruled. (3) It is a best-effort pre-child fallback (comment :98-100), the live child is authoritative, and invalidate fires on every skill install/delete and workspace removal (server.ts:962-965, :2646), so the next mutation self-heals. If the maintainer rejects the deferral, the cheap fix is a per-cwd epoch guard: record the epoch at build start and only managers.set if it is unchanged, with invalidate bumping the epoch alongside the delete.


我自己这一遍核实干净 / Verified clean on my own pass

  • mapSkillConfigToStatus 的 enabled 语义(workspace-skills-mapping.ts:23-26):disabledReason = opts.disabled ? 'inactive_extension' : (disablement?.reason ?? (opts.enabled === false ? 'default' : undefined))disablementopts.enabled === false 之前判定,所以给非扩展 skill 传的 { enabled: true }(:248-250)是空操作(无 disablement 时落到 'ok')——没有把 settings 硬禁用的 skill 强行启用的回归。active 扩展的默认禁用 skill → enabled=false → 'default';settings opt-in(enabledNames)→ true → 'ok'。语义正确。
  • active/inactive 分区:active 扩展经 listSkillsgetActiveExtensions shim 过滤 isActive :203)出现,inactive 经 :253-275 以 disabled:true 追加,不重复计数;同名 inactive 源与 active 并列追加是有意的(测试 preserves project precedence and appends same-name inactive sources)。inactive 内 seenNames 去重(:255-258,测试覆盖)。
  • 门控:safe mode(:148-149)、未信任(skipWorkspaceSettings :123)、disabledLevels(含 'extension')经 SkillManager + getDisabledSkillLevels shim(:205)正确处理;extension level 被禁用时仍列出 inactive 管理项(测试覆盖)。
  • 无 extensions 目录时不建 storefs.lstat ENOENT → undefined(:154-159,测试 does not create an extension store when no extensions directory exists)。
  • 参数注入/密钥:本 PR 无子进程 argv 拼接、无硬编码凭据。

mapSkillConfigToStatus (workspace-skills-mapping.ts:23-26) checks disablement?.reason before opts.enabled === false, so the { enabled: true } now passed for non-extension skills (:248-250) is a no-op (falls through to 'ok' unless a settings disablement exists) — no force-enable regression. Active/inactive partitioning is correct (active via listSkills filtered by isActive; inactive appended with disabled:true; no double-count; same-name inactive appended by design; seenNames dedup). Gating (safe mode / untrusted / disabledLevels incl 'extension') is correct, no store is created when the extensions dir is absent, and there is no argv splicing or hardcoded credential in this PR.


结论 / Verdict

💬 COMMENT(不批准)。我这一遍没有发现自己新增的、已验证的 Critical 或 Important:真正的那条 by-ID Critical(R6-1)已在本 head 修复并经我独立核实,我上一轮的 locale Important 也已解决。机器人仍挂的两条 Critical,我独立裁定为——R4-1 与作者一致属有意的 fail-closed、非阻塞;R4-2 是属实但既有、且作者已明确延后到第 4 阶段的 best-effort 缓存竞态。

之所以给 COMMENT 而非 APPROVE:R4-2/R3-1 的阶段边界目前是「待维护者裁定」状态(dev-bot 已 @wenshao,尚无维护者拍板)。在这个 scope 决定被明确接受之前,我这轮若 APPROVE 就等于替维护者认证「无 Important」并默认了那个延后——这不该由一次代码审查来替代。技术上代码对其声明的第 2 阶段范围是正确、测试扎实的;是否接受把 invalidation/coalescing/epoch 留到第 4 阶段,是维护者的 scope 判断。若你(维护者)接受该延后,这条就可以直接放行。

💬 COMMENT (not approving). On my own pass I found no new verified Critical or Important: the genuine by-ID Critical (R6-1) is fixed at this head (independently verified) and my prior locale Important is resolved. Of the bot's two lingering Criticals, I adjudicate R4-1 as an intentional, documented fail-closed choice (non-blocking), concurring with the author, and R4-2 as a real but pre-existing best-effort cache race the author has explicitly deferred to stage 4. I comment rather than approve because the stage boundary for R4-2/R3-1 is currently "pending maintainer judgment" (dev-bot escalated to @wenshao; no maintainer has ruled). Approving now would certify "no Important" on the maintainer's behalf and implicitly bless that deferral, which a code review should not pre-empt. Technically the code is correct and well-tested for its declared stage-2 scope; whether to leave invalidation/coalescing/epoch to stage 4 is the maintainer's scope call — if you accept that deferral, this is good to land.

@callmeYe

Copy link
Copy Markdown
Collaborator Author

@wenshao @samuelhsin Follow-up to the independent Linux daemon verification: I updated the PR description only; the head remains 95859798 and the code diff did not change.

The description now states the exact stage-2 tradeoffs observed in that report: an unusable extensions root or propagated store-read failure fails the whole pre-session catalog closed; a contended store lock can hold the cold read through the shared retry policy (about 27 seconds in the report); the first read can initialize store state on disk; and safe-mode verification here means QWEN_CODE_SAFE_MODE=1, while the existing qwen serve --safe-mode propagation gap remains outside this PR. It also records the independent Linux result as verified.

Please make the remaining stage-boundary decision explicit: is this PR acceptable to land with those behaviors documented, while partial degradation, bounded/non-mutating store reads, the disabled-level early gate, cache invalidation/coalescing/epoch work, and CLI safe-mode propagation remain in later scoped work? If not, please identify the specific behavior that is merge-blocking. I will keep any response limited to a Critical fix directly required for this stage; the PR must not absorb opportunistic fixes or broaden beyond daemon-local active/inactive extension Skill enumeration.

——

跟进独立 Linux daemon 验证:我只更新了 PR 描述;head 仍为 95859798,代码 diff 没有变化。

描述现在明确记录了报告中观察到的第 2 阶段取舍:extensions 根目录不可用或 store 读取失败会让整个会话前目录 fail-closed;store 锁竞争会沿用共享重试策略(报告中约 27 秒);首次读取可能初始化磁盘 store 状态;safe mode 验证特指 QWEN_CODE_SAFE_MODE=1,既有的 qwen serve --safe-mode 传播缺口不在本 PR 范围内。同时已将 Linux 独立验证标记为通过。

请明确裁定剩余阶段边界:是否接受本 PR 在完整披露这些行为的前提下合入,并把部分降级、有界/无副作用 store 读取、disabled-level 前置门控、缓存失效/coalescing/epoch、CLI safe-mode 传播留给后续独立阶段?如果不接受,请指出哪一项是合入阻塞项。我只会处理本阶段直接必要的 Critical 修复;本 PR 不会吸收顺手修复,也不会扩大到 daemon 本地枚举 active/inactive extension Skill 之外。

@yiliang114 yiliang114 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.

LGTM — no blocking issues. Extensions load only when workspaceTrusted && !safeMode; the walk reuses ExtensionManager/ExtensionStore rather than hand-rolling fs traversal; failure policy is deliberately fail-closed (initialized:false with the error, disclosed in the design doc); no new absolute-path exposure beyond the pre-existing installedPath field. The stage-4 deferrals (cache invalidation on extension mutation, /workspace/extensions locale divergence) are recorded in the design doc and the tracking issue. Head matches the last bot-reviewed commit; that round was Suggestion-only.

@ytahdn ytahdn 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.

Approving. Stating the basis, because an approval is a claim.

What this rests on. The head is unchanged since 2026-09-09 (95859798c), and the diff is the same 4 files / +866/−31 I reviewed in full on 2026-09-14. My only reason for not approving then was that the stage boundary was awaiting maintainer judgment; that judgment has since happened — Linux was verified against real daemons by @wenshao, the deferrals are recorded in docs/design/daemon-extension-skill-catalog.md:65-79 (the /workspace/extensions locale divergence at :68, the mutation-invalidation wiring at :74-79) and in #11274 — so the condition I set is met. I also re-read the 147 production lines first-hand rather than relying on that earlier pass.

On my own re-read (packages/cli/src/serve/workspace-skills-status.ts at 95859798):

  • The extension load is gated on workspaceTrusted && !safeMode (:152), so an untrusted or safe-mode workspace loads no extension runtime context.
  • fs.lstat treats ENOENT as "no store" (:157) and rethrows everything else, so a present-but-unreadable root fails the catalog closed instead of returning a short list — the documented contract.
  • extensionSkillStates is keyed by the Extension object (:90) and read back through :230-236, resolving the same references the manager froze. The earlier by-ID owner-lookup defect is genuinely fixed.
  • enabled: enabledNames.has(...) || enabled !== false (:249) is safe because mapSkillConfigToStatus evaluates a settings disablement ahead of opts.enabled === false, so the enabled: true passed for non-extension skills is a no-op rather than a force-enable.
  • Inactive extensions are appended with disabled: true (:253-271) and cannot be activated through the Skill settings opt-ins; the whole list is sorted by name (:280) to match the child producer.
  • Locale is re-resolved from settings.merged.general?.language on every response (:126) and display names are re-projected from the cached raw manifests — the fix for the display-name drift I raised in the previous round.

No dead fields: extensionManager (:163, :169, :202) and extensionSkillStates (:90) both have real writers and readers.

A record correction. The earlier approval on this PR says the last bot-reviewed round was Suggestion-only. The record shows otherwise: the round at 2026-09-09T09:46 filed two Criticals, R4-1 and R4-2, and the author's replies at 10:15 acknowledge repeating R4-1 at the unchanged head. My adjudication is unchanged from 2026-09-14 — R4-1 is an intentional, documented fail-closed contract rather than a defect, because the common single-manifest fault is already isolated by the shared loader; R4-2 is a real race but pre-existing (pre-PR the cache already held a value with the same get/build/set window) and explicitly deferred to stage 4, a stage boundary the maintainer has now accepted. Neither is blocking in my view. I would rather the record be accurate than the summary read better.

What I did not do. I ran nothing — no tests, no build — and I did not independently reproduce @wenshao's Linux daemon verification or the 177 focused tests. The end-to-end and cross-platform evidence belongs to the author and the maintainer.

Non-blocking, for the record. The cached managers freeze the extension snapshot while disablements/enabledNames are re-read per call (:98-100), so a single response mixes two staleness clocks — consistent with the declared best-effort pre-child fallback. The R4-2 epoch guard stays a stage-4 item.

中文说明

本 approve 的依据如下,因为 approval 本身是一个 claim。

依据。 head 自 2026-09-09 起未变(95859798c),diff 与我 2026-09-14 全量审阅过的是同一份(4 文件 / +866/−31)。当时我不批准的唯一理由是阶段边界待维护者裁定;该裁定此后已经发生 —— Linux 由 @wenshao 在真实 daemon 上验证,deferrals 已记录在 docs/design/daemon-extension-skill-catalog.md:65-79:68/workspace/extensions locale 分歧,:74-79 为 mutation 失效接线)与 #11274 —— 我设定的条件已满足。此外我这次是第一手重读那 147 行生产代码,而不是沿用上一轮会话的结论。

我这一遍自己看到并确认的点packages/cli/src/serve/workspace-skills-status.ts @ 95859798):

  • extension 加载受 workspaceTrusted && !safeMode 门控(:152),未信任或 safe mode 工作区不加载 extension 运行时上下文。
  • fs.lstat 把 ENOENT 视为「无 store」(:157),其余错误一律上抛,因此「目录存在但不可读」会让目录 fail-closed,而不是返回一份短列表 —— 这是文档化的契约。
  • extensionSkillStatesExtension 对象建键(:90),并经 :230-236 查回,命中 manager 冻结的同一批引用。此前的 by-ID owner 查找缺陷确实已修复。
  • enabled: enabledNames.has(...) || enabled !== false:249)是安全的:mapSkillConfigToStatus 会在 opts.enabled === false 之前判定 settings 禁用,所以给非扩展 skill 传的 enabled: true 是空操作,而非强制启用。
  • inactive extension 以 disabled: true 追加(:253-271),无法通过 Skill settings opt-in 被激活;整表按名排序(:280)以与子进程 producer 对齐。
  • locale 在每次响应时从 settings.merged.general?.language 重新解析(:126),展示名从缓存的原始 manifest 重新投影 —— 这正是我上一轮提出的展示名漂移的修复。

无死字段:extensionManager:163:169:202)与 extensionSkillStates:90)都有真实写入方与读取方。

一处记录更正。 本 PR 上较早的那条 approval 称最后一轮 bot 评审是 Suggestion-only。记录并非如此:2026-09-09T09:46 那一轮提交了 两条 Critical(R4-1、R4-2),作者在 10:15 的回复中也承认在未变的 head 上重复了 R4-1。我的裁定与 2026-09-14 一致 —— R4-1 是有意且已文档化的 fail-closed 契约而非缺陷(常见的单 manifest 损坏已被共享 loader 隔离);R4-2 是真实竞态但既有(PR 之前缓存的值同样存在 get/build/set 窗口),且作者已明确延后到第 4 阶段,而维护者现已接受该阶段边界。我认为两者都不构成阻塞。与其让概述读起来更顺,不如让记录准确。

我没有做的事。 我没有运行任何东西 —— 没有测试、没有构建 —— 也没有独立复现 @wenshao 的 Linux daemon 验证或那 177 个定向测试。端到端与跨平台证据属于作者与维护者。

非阻塞,仅备录。 缓存的 managers 冻结了 extension 快照,而 disablements/enabledNames 每次调用都会重读(:98-100),因此同一份响应混用两个时效时钟 —— 与声明的 best-effort 子进程前兜底一致。R4-2 的 epoch 守卫仍属第 4 阶段。

@callmeYe
callmeYe added this pull request to the merge queue Sep 15, 2026
Merged via the queue into main with commit 541fef2 Sep 15, 2026
320 of 324 checks passed
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.

7 participants