fix(cli): restore shared Skill status and runner CI checks - #11933
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical — and I confirmed it by reading the code rather than taking the description's word for it. Core's Direction: aligned. This restores parity between two producers that Size: not applicable — no core paths. Approach: the scope feels right, and it's the fix I'd have written. It reuses Two things worth a look before this goes further, neither a blocker:
Risk: no elevated risk signals — none of the changed paths match the revert-correlated set. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 是已观测到的 bug，不是理论性加固——而且我是通过读代码确认的，没有只采信 PR 描述。Core 的 方向： 对齐。这个改动恢复的是两个 producer 之间的一致性，而 规模： 不适用——未触及核心路径。 方案： 范围合理，也正是我会写的修法。它复用了 core 的 有两点在继续深入前值得看一下，都不是阻塞项：
风险： 无升级风险信号——改动路径均未命中与 revert 相关的集合。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
Code reviewBefore reading the diff I wrote down what I'd do from the title and the "Why it's needed" section alone: the fallback's No critical blockers and no AGENTS.md violations. Specifics I checked rather than assumed:
Two non-blocking observations, named rather than gated on: The inactive-row construction is now duplicated verbatim between Separately, and echoing Stage 1: the renamed test now expects three rows where it expected two, because CI test evidenceThis is the PR's own CI read through the API. I built and executed nothing from this branch — the review is static, so every number below is GitHub's, not mine. Thirty-five check runs on the reviewed commit, zero failures and zero cancellations. Everything that has settled is green: The honest gap: the three checks that actually gate this change are still running — So the PR's "42/42 workspace Skill tests, 8/8 status mapping, 30/30 settings, 6/6 runner tests in each module mode" figures are the author's claim, not independently re-run, and its own "Tested on" table lists Linux as awaiting hosted CI. What I can attest to from reading the code is the mechanism: the lookup miss is real,
Sandboxed verification would settle the part static review cannot: 中文说明代码审查 在读 diff 之前，我先只根据标题和"为什么需要"写下了自己的修法：fallback 的 没有阻塞性问题，也没有违反 AGENTS.md。以下是我实际核对过、而非想当然的点：
两点非阻塞的观察，只点名、不作为门槛： 未激活条目的构造现在在 另外，与 Stage 1 呼应：重命名后的测试现在期望三行而非两行，因为 CI 测试证据 以上是通过 API 读取的本 PR 自身 CI。我没有构建或执行该分支的任何代码——本次审查是静态的，因此下面每个数字都来自 GitHub，不是我自己跑出来的。 被审 commit 上共 35 个 check run，零失败、零取消。已经出结果的全部为绿： 需要如实说明的缺口：真正能判定本次改动的三个 check 仍在运行—— 因此 PR 中"工作区 Skill 42/42、状态映射 8/8、设置 30/30、runner 两种模式各 6/6"这些数字属于作者自述，未经我独立复跑，其"测试环境"表格本身也把 Linux 标为等待远端 CI。我能通过读代码确认的是机制：查询落空确实存在， 沙箱验证可以补上静态审查覆盖不到的部分： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the production fix is six lines that I could verify against two independent authorities in the tree, and it closes a fail-open on a permission-adjacent path; the reservations are all non-blocking and mostly about what's bundled alongside it. Stepping back: this is a real regression fix, not a hypothesis. Core's What gives me the 4 is that I didn't have to take the PR's word for the mechanism. The fix mirrors I also checked the diff is complete rather than trusting the file list — the neighbouring suites already assert qualified names or stub around Where I land on the three reservations, none of which I'd gate on: The bundled runner-fixture change is scope creep and I'd still prefer it split out. It's also the one claim in this PR I could not check at all: its justification is The duplicated inactive-row construction between the two producers is the thing I'd actually want addressed, because it's the root cause of this class of drift, not a symptom. But extracting it is a separate change with its own blast radius, and doing it inside a six-line regression fix would make this harder to review and revert. Follow-up issue territory. And the cross-level precedence shift — a project Skill and an extension Skill sharing an authored name now both surface — is a genuine behaviour change that this PR documents but does not cause. It arrives with core's naming change. If that was unintended, it needs a decision in core, not a patch here. On the pattern question, since this author has ten PRs open and six landed today: I evaluated this one on its own merits and it holds up — small, focused, verified against the code, and it unblocks CI for three of the author's other PRs rather than advancing a feature. The volume is worth a maintainer's awareness in the aggregate, but it isn't a reason to discount a PR that fixes a red baseline. Approval is deferred until CI lands green on 中文说明Confidence: 4/5 —— 生产代码修复只有六行，而且我能拿代码树里两处相互独立的权威实现来验证它；它堵住的是一个权限相关路径上的 fail-open。保留意见全部是非阻塞的，且大多与"顺带打包进来的东西"有关。 退一步看整体：这是一次真实的回归修复，不是假设。Core 的 给我 4 分的依据是：机制部分我不必采信 PR 的说法。这个修复在归属查询上对齐了 我还核对了 diff 的完整性，而不是直接采信文件清单——相邻套件要么已经在断言完整名，要么用桩数据绕过了 关于三条保留意见，我的结论是：都不足以作为门槛。 打包进来的 runner 测试替身改动属于范围蔓延，我仍然更希望它被拆出去。它也是本 PR 中我唯一完全无法核实的说法：其依据是 两个 producer 之间重复的未激活条目构造，才是我真正希望被处理的一点，因为它是这类漂移的根因，而不是症状。但把它抽出来是一次独立的改动，有自己的影响面；塞进一个六行的回归修复里，只会让本次改动更难审查、更难回滚。这属于后续 issue 的范畴。 至于跨层级优先级的变化——同名的 project Skill 与 extension Skill 现在都会出现——是一个真实的行为变化，本 PR 记录了它但并未造成它。它随 core 的命名改动一起到来。如果这不是有意为之，需要在 core 里做决定，而不是在这里打补丁。 关于"是否属于某种模式"这个问题，鉴于该作者有 10 个 PR 处于开启状态、今天就有 6 个：我是就这一个 PR 本身的价值来评估的，而它站得住——小、聚焦、经代码核实，并且它是为作者另外三个 PR 打通 CI，而不是推进某个功能。整体上的数量值得维护者留意，但这不足以让我折价看待一个修复红色基线的 PR。 批准将推迟至 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
main's #11933 (faded7f) landed the identical workspace skill identity fix: packages/cli/src/serve/workspace-skills-status.ts merges to the same blob on both sides, and main's workspace-skills-status.test.ts is a superset of this branch's version (this branch deleted main's `scopes grants to the owning extension` cases). Both files are therefore taken from origin/main, which leaves the two corrected .size-baseline integers as the whole diff. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Picks up main's review-runner-schedule helper-test fix (#11933), which writes a commonjs package.json into the fake-gh temp dir so the Lint lane's helper tests stop failing with 'require is not defined in ES module scope' when TMPDIR resolves inside the repo. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmu2luc9n6d
Picks up #11933, which aligned packages/cli/src/serve/workspace-skills-status.test.ts with the qualified Skill identities the provider already emits. This branch never touched that file; its PR base predates the fix, so the Test lane ran the stale expectations and reported 13 failures. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmu2luc9n6d
Picks up #11933, which aligned packages/cli/src/serve/workspace-skills-status.test.ts with the qualified Skill identities the provider emits. This branch never touched that file, so the 13 Test-lane failures were the stale expectations from its older base, not a regression here. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmu2luc9n6d
4 participants
What this PR does
Restores extension Skill defaults and workspace overrides in the daemon's fallback catalog after Skill names gained extension prefixes. Inactive entries now use the same qualified identity as the live runtime. Also makes the runner-scheduling test fixture explicitly CommonJS so it works when Node defaults to ESM.
Why it's needed
The shared baseline produces 13 workspace Skill test failures, including disabled Skills incorrectly reported as enabled. The runner helper also fails two tests under an ESM default. These failures affect #11922, #11924 and #11927 independently of their Goal changes.
Reviewer Test Plan
How to verify
NODE_OPTIONS=--experimental-default-type=module; both should pass.Evidence (Before & After)
Baseline reproduction: 13/39 workspace Skill tests failed; the runner helper failed 2/6 tests under an ESM default. After the fix: 42/42 workspace Skill tests, 8/8 status mapping tests, 30/30 settings tests, and 6/6 runner tests in each module mode pass. Build, bundle, repository typecheck, targeted ESLint and formatting checks pass. No visual UI change.
Tested on
Risk & Scope
Linked Issues
Related: #11922, #11924, #11927.
中文说明
改动
修复 Skill 名称增加扩展前缀后，daemon fallback 目录未正确读取扩展默认值和工作区覆盖配置的问题。未激活条目现在使用与运行时一致的完整名称。同时明确 runner 调度测试替身使用 CommonJS，使其在 Node 默认 ESM 模式下也能运行。
原因
公共基线存在 13 个工作区 Skill 测试失败，其中包含禁用 Skill 被错误报告为启用的实际行为偏差。Runner 辅助测试在默认 ESM 模式下另有 2 个失败。这些问题影响 #11922、#11924、#11927，与它们的 Goal 改动相互独立。
验证方法
NODE_OPTIONS=--experimental-default-type=module下运行 runner 调度测试，均应通过。验证证据
修复前：工作区 Skill 测试 13/39 失败，runner 测试在默认 ESM 模式下 2/6 失败。修复后：工作区 Skill 42/42、状态映射 8/8、设置 30/30 通过；runner 测试两种模式均为 6/6。Build、bundle、全仓 typecheck、相关 ESLint 和格式检查通过。没有视觉 UI 改动。
测试环境
风险与范围
关联：#11922、#11924、#11927。