fix(core): validate skill commands against the live provider to survive late attach - #9824
Conversation
…ve late attach SkillTool caches the model-invocable command set during construction, but in interactive mode the provider is only registered after CommandService initialisation resolves — after Config.initialize() has already warmed the tool registry. The cache then stays empty until an unrelated SkillManager change event re-runs refreshSkills(), so validateToolParams intermittently rejects commands announced by the per-turn skills drain (issue QwenLM#9821). Consult the synchronous provider live in validateToolParams instead, applying the same file-based-skill name shadowing as collectAvailableSkillEntries so path-gated skills stay gated. The execute path already reads the executor live. Regression tests cover late attach, paths gating, and the no-provider SDK path.
|
Thanks for the PR — re-running the gate on the current head after five Template still good ✓ Problem: still an observed bug with solid evidence — #9821 is open, Direction: aligned — a correctness fix for an intermittent user-facing failure on the core Skill-tool path. Size: production logic ≈ 115 lines (config.ts +5, skill-utils.ts +6, skill.ts +100/−4); test lines 250 (skill.test.ts). The diff grew since the first gate pass (≈ 61 production lines then): the additions are the hidden-skill/command-collision guard that round-2 review flagged as Critical, plus its coverage. Well under any threshold. Approach: scope still feels right. The core fix (live provider read at validation time, cache as fallback) is unchanged, and the collision guard sits on the same validation/execute path — it closes the case where invoking a command that shares its name with a Risk: no elevated risk signals — no files on the revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献——五轮 模板依然完整 ✓ 问题：仍是证据充分的已观测 bug——#9821 仍 open，带 方向：对齐——核心 Skill 工具路径上用户可见间歇性失败的正确性修复。 规模：生产逻辑约 115 行（config.ts +5、skill-utils.ts +6、skill.ts +100/−4）；测试 250 行（skill.test.ts）。相比首次门检（约 61 行生产代码）diff 有所增长：新增部分是第 2 轮 review 标记为 Critical 的隐藏技能/命令同名冲突防护及其测试覆盖。远低于任何阈值。 方案：范围仍然合理。核心修复（校验时实时读取 provider、缓存兜底）未变；冲突防护位于同一条校验/执行路径上——它关闭了调用与 风险：无升级风险信号——未命中与 revert 相关的路径清单。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo Critical findings on this head — the round-2 Critical is resolved, and round 5 of
Open by author deferral, recorded but not blocking: single-sourcing the shadow rule (R1-1), extracting the shared executor fallback (R3-2), required Test evidence — the PR's own CI (review is static; no PR code was executed here)The workflow-size gate failure that blocked the earlier head is resolved on main, so CI ran the full suite this time and is entirely green on this commit — notably
The regression tests pin the fix mechanically: they construct What unit CI cannot settle is the live-session claim itself — that an interactive TUI run with user-level slash commands no longer hits 中文说明代码审查当前 head 无 Critical——第 2 轮的 Critical 已解决，第 5 轮
作者明确推迟、记录但不阻塞：遮蔽规则单一来源（R1-1）、共享 executor 兜底抽取（R3-2）、 测试证据——本 PR 自己的 CI（审查为静态，未执行任何 PR 代码）阻塞早前 head 的工作流体积门禁失败已在 main 上解决，本次 CI 完整运行且在该提交上全绿——尤其是 （CI 结论表格见上方英文部分） 回归测试在机制上钉住了修复：用例以 null provider 构造 单测 CI 无法覆盖的是真实会话声明本身——交互式 TUI 下用户级 slash 命令不再出现 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the fix is proven rather than plausible (shadow-set equivalence, a real collision closed by the guard, regression tests that pin the change, fully green CI); docking one point only because the live-TUI end-to-end claim still rests on the author's word until the in-flight verify job reports. Stepping back: this PR aged well across its five review rounds. The core fix is exactly what I'd have written — validation reads the provider live, the cache stays as fallback, and the shadowing semantics are provably identical to the announce side. The rounds added only what earned its place: the round-2 Critical was a real hole (a same-named command silently executing a The one thing I still cannot attest to is behavior in a live interactive session. Unit CI pins the mechanics — the regression sequence fails with the exact reported signature on the old code — but the race only manifests under real 中文说明回顾整体：这个 PR 在五轮 review 中经受住了检验。核心修复正是我会写的样子——校验时实时读取 provider，缓存保留为兜底，遮蔽语义与宣告侧可证明完全一致。各轮只增加了配得上的内容：第 2 轮的 Critical 是真实漏洞（同名命令会悄悄执行 唯一仍无法背书的是真实交互会话中的行为。单测 CI 钉住了机制——回归序列在旧代码上会以与报告完全一致的签名失败——但竞态只在真实 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 94 passed — this review observed 21013, 1685, 23494, 1653, 495, 4169, 599 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：94 passed — this review observed 21013, 1685, 23494, 1653, 495, 4169, 599 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Closeout for f3383d3: fixed the hidden-skill/command collision so |
|
Follow-up for 9e7b089: fixed the CI TypeScript failure by defaulting optional hidden-skill collection data for existing test mocks. Prettier and |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
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 5": running packages/core/src/tools/skill.test.ts to confirm the new tests are green — the worktree and the parent checkout have no node_modules or built dist …; "agent 6a": none — but note I did not run the test suite (verification stage's job) and did not trace telemetry consumers of SkillLaunchEvent beyond confirming the call-s….
Test Plan (not a blocker): 94 passed — this review observed 21049, 1685, 23525, 1654, 495, 4190, 610 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/skill.ts:296 — [review] R1-1 still stands — shadow set re-implements the collect-side name-shadowing rule; author explicitly deferred single-sourcing beyond the #9821 fix
Convergence: round 3 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 4 (1 new). Findings keep coming back to the same files: packages/core/src/tools/skill.ts (findings in rounds 1, 2; 3 more now). The rate of new findings is not falling. 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. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 5"：running packages/core/src/tools/skill.test.ts to confirm the new tests are green — the worktree and the parent checkout have no node_modules or built dist …；"agent 6a"：none — but note I did not run the test suite (verification stage's job) and did not trace telemetry consumers of SkillLaunchEvent beyond confirming the call-s…。
Test Plan（非阻断）：94 passed — this review observed 21049, 1685, 23525, 1654, 495, 4190, 610 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 4 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/core/src/tools/skill.ts（第 1、2 轮已出过发现，本轮又有 3 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Closeout update: fixed the hidden-skill command fallback terminal failure path by logging failed launches, preserving failed-invocation attribution when no command fallback exists, warning on swallowed executor errors, and adding hidden-guard coverage for executor error/throw/null/no-executor exits. Intentionally not changed: shared executor helper extraction and required hiddenSkillNames typing, both left out to avoid widening this bugfix. Verification: Prettier check and git diff --check passed; focused Vitest could not run locally because this isolated worktree dependency state cannot resolve ajv/dist/2020.js after the build prerequisite step. |
|
Closeout update: refreshed this fork branch with latest main to recover the exact-head CI failure. No conflicts; no product code changed beyond the merge. Verification is pending on the new CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-2 hidden-skill guard remains a ~40-line near-verbatim copy of the disabled guard — already reported (comment 3840692075); author declined extraction in this bugfix
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/core/src/tools/skill.ts (findings in round 3; 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. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛情况：第 4 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/core/src/tools/skill.ts（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Closeout update: added hidden-skill fallback coverage for forwarding command args so same-named prompt delegation keeps user-supplied arguments. Verified with Prettier, git diff --check, full build, and focused core Vitest for the hidden-skill cases. New CI is pending on the pushed head.\n\n |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/skill.test.ts:1558 — [review] hidden-skill test inlines the createHiddenSkillInvocation helper setup instead of calling itpackages/core/src/tools/skill.test.ts:1592 — [probe] hidden-skill happy-path test missing recordSkillInvocation assertion (telemetry mutant survives 93/93)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Local runtime verification: PASS (late-attach race reproduced end-to-end)
Before excerpt: After excerpt: So a command announced in Visual evidence (terminal screenshots, same driver on each arm): Before (merge-base
After (PR head
|
1 similar comment
|
Local runtime verification: PASS (late-attach race reproduced end-to-end)
Before excerpt: After excerpt: So a command announced in Visual evidence (terminal screenshots, same driver on each arm): Before (merge-base
After (PR head
|
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 38 passed · 0 failed · 38 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：38 通过 · 0 失败 · 38 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #9824 verification —
|
| cell | oracle | head (f1defaa) | base (d128998) |
|---|---|---|---|
| A1 | SkillTool constructed by initialize | PASS | PASS |
| A2 | real skill discovered from disk (validity control) | PASS | PASS |
| A3 | pre-attach validate('late-command') |
rejected (not-found) | rejected (not-found) |
| A4 | post-attach validate('late-command') — THE FLIP |
null (accepted) |
not-found string (bug) |
| A5 | negative control never-registered |
rejected | rejected |
| A6 | real skill still validates post-attach | null |
null |
| A7 | validate('hidden-cmd') post-attach |
null |
not-found (same stale-cache bug) |
| A8 | execute('hidden-cmd') with executor — FLIP 2 |
runs executor (EXECUTED:hidden-cmd:hello, "Delegated to command") |
loads hidden skill body (bug) |
| A9 | positive control: visible skill body executes | PASS | PASS |
Both arms 9/9 against their arm-specific expectations (base expectations encode the buggy behavior, so a green base arm proves the contrast).
Cell set 2: vitest red/green — HEAD's skill.test.ts copied into the base worktree (compiles against base sources; only public API used). Witness: evidence/02-vitest-red-green.png.
| run | result |
|---|---|
base + head tests, -t "issue #9821" |
2 failed / 2 passed — failures are the two late-attach tests with the exact reported signature expected 'Skill "late-command" not found. Available skills: code-review, testing' to be null |
head, -t "issue #9821" |
4 passed |
| base + head tests, full file | 8 failed / 85 passed — exactly the 2 late-attach tests + the 6 hidden-skill execute tests; no collateral |
Cell set 3: mutation matrix (mutants applied to a scratch HEAD worktree, full skill.test.ts each, file restored after). Witnesses: evidence/03-mutant-m1-cache-only.png, evidence/04-mutant-m4-no-hidden-guard.png; raw per-mutant logs in logs/mutant-m*.txt.
| mutant | change | result | caught by (intended assertion?) |
|---|---|---|---|
| M0 | none (control) | 93/93 green | — |
| M1 | live read → cache only (2 call sites) | 2 failed | the two late-attach tests — vacuity proof of the central claim |
| M2 | delete throw-fallback | 1 failed | "falls back to cached commands when the live provider throws" |
| M3 | delete name-shadowing filter | 2 failed | new late-gating test (validate returned null where a gated message was expected) + pre-existing "does not allow a pending conditional skill to be invoked via the command path" |
| M4 | delete hidden-skill execute guard | 6 failed | all 6 hidden-skill execute tests (loadSkillForRuntime spy called with the hidden name) |
| M5 | delete hidden-branch recordSkillInvocation |
1 failed | "returns not-found and records failure when no hidden skill command alternative exists" |
| M6 | change empty-state not-found wording (positive control, same file) | 2 failed | the two tests pinning that exact string (skill.test.ts:415 + initialization degrade test) |
| M7 | drop hiddenSkillNames from collectAvailableSkillEntries result |
6 failed | same 6 hidden tests — the skill-utils.ts hunk is load-bearing; ?? new Set() keeps the chain type-safe but empty |
No survivors → no coverage gaps, no dead guards. M4 and M7 kill the same six tests: the hidden-skill hazard is closed by a two-hunk chain (collect → cache → execute), each hunk individually necessary.
Reviewer Test Plan walkthrough
| plan step | result |
|---|---|
1. Red-before-fix, -t "issue #9821" on tests-only |
reproduced exactly (cell set 2, row 1) |
| 2. Green with fix, 4 regression tests | reproduced (cell set 2, row 2) |
| 3. Targeted suites "94 passed" | green at merge head with 102 passed (93 skill + 9 skill-utils static its expand to 102 with it.each); the delta vs 94 is main-side test drift merged after the PR's count was written — the PR's own contribution is exactly +12 it blocks (78→90 in skill.test.ts), consistent |
4. tsc --noEmit on core |
exit 0, 0 errors (witness evidence/05-targeted-gate-typecheck.png) |
| 5. eslint/prettier clean on changed files | both clean; both gates proven live by planted violations (unused var → eslint error; malformed spacing in packages/core/src/tools/ → prettier warn) |
Findings
No blocking findings. Non-blocking observations:
- Optional
hiddenSkillNamesfield is redundant defence.CollectedAvailableSkills.hiddenSkillNames?is optional andrefreshSkillsguards with?? new Set(), but the only producer (collectAvailableSkillEntriesUncached) always sets it. M7 shows the chain matters; the??itself defends only the type contract. Correct as-is. - Live-read shadowing uses cached skill sets.
getModelInvocableCommands()filters live commands againstavailableSkills/pendingConditionalSkillNamesfrom the lastrefreshSkills(). A file-based skill created between refreshes would not shadow a same-named command until the SkillManager watcher fires. Window is bounded by the existing watcher mechanism and identical to the pre-existing 2 s collect memo; not a regression, noted for completeness. - Description count drift ("94 passed" vs 102 at merge head) — main-side drift, arithmetic consistent with the PR's +12 tests; not a defect.
Not covered
- Per-commit attribution: checkout is depth-2 (merge, base tip, PR head only);
git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary while the metadata lists 7 commits. Verified the aggregateHEAD^1..HEADdiff only. - Full
packages/coresuite: ranskill.test.ts,skill-utils.test.ts(102) andenvironmentContext.test.ts(64) at head, plus the base-side census; the giantclient.test.ts/coreToolScheduler.test.tssuites mockcollectAvailableSkillEntriesand were not run. - Live interactive TUI E2E: the mock-free harness drives the real startup path (
Config.initialize→warmAll→ late provider attach) but not a full TUI session; the PR also declares this out of scope. - Base-tree typecheck: not cited — my worktrees reused the root
node_modulesplus a symlinked package-localnode_modules(external deps only; asserted no@qwen-code/*workspace links inside it), which produced a single@lydell/node-ptyTS7016 resolution artifact absent in the main tree (0 errors). Environmental to my harness, not to the PR. - Windows/macOS (PR declares Linux-only).
Methodology
Environment: node:22-bookworm-class container, Node v22.23.2, no GitHub token. Arms: scratch git worktrees at HEAD^1 (base) and HEAD (mutant/head), each wired to the pre-installed root node_modules plus a symlink of packages/core/node_modules (verified external-deps-only, so no workspace code crosses the tree boundary; the code under test always resolved within its own tree via relative imports). Harnesses: harness/mockfree-ab.mts (real objects, tsx), harness/mutate.sh (7 mutants), harness/capture-all.sh (5 verify-capture.mjs PNGs in evidence/). Raw logs in logs/. Assertion counts in assertions.json map 1:1 to the cells/rows above; expected base-arm reds are encoded as passing assertions.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/tools/skill.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/skill.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/tools/skill.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/tools/skill.test.ts: P (exit 0)
round 2 · packages/core/src/tools/skill.test.ts: P (exit 0)
round 3 · packages/core/src/tools/skill.test.ts: P (exit 0)
round 4 · packages/core/src/tools/skill.test.ts: P (exit 0)
round 5 · packages/core/src/tools/skill.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Scope: Standard tier — 4 files, 491 lines diff (mostly new tests).
Checked:
- Root cause:
SkillToolconstructor callsrefreshSkills()beforeCommandServiceregisters themodelInvocableCommandsProvider. The fix reads the live provider on everyvalidateToolParamscall viagetModelInvocableCommands(), falling back to the cached set when the provider is absent or throws. - Shadow filter:
getModelInvocableCommands()filters commands whose names collide withavailableSkillsorpendingConditionalSkillNames— preserving the path-gating branch. Testkeeps path-gated skills gated when the provider is late-registeredpins this directly. - Hidden skill execute path:
isSkillHiddenis checked first inexecute(). Command executor is tried; success returns the MCP result; throw/null falls through to not-found. All outcomes covered by the 5 new hidden-guard execute tests. - Prior critical R2-1: "command with same name as hidden skill passes validation" — this is the intended behavior; the test
should accept a command with the same name as a hidden file skillexplicitly validates it, and the execute path routes to the command executor correctly. Confirmed not a blocker. - Error handling:
catchingetModelInvocableCommands()logs viadebugLogger.warnand falls back to cache — correct resilience.
CI: precheck-pr SKIPPED — fork PR security model.
Cross-check: R2-1 was addressed (intentional design, confirmed by test). R3-x suggestions (shadow-set refactor, arg-forwarding test, telemetry on throw) are non-blockers. Latest prior APPROVED covers current head.
Reviewed with AI assistance.
Local deep verification —
|
| arm | late-attach validation of late-command |
hidden skill + same-named command at execute() |
|---|---|---|
base 3892ca32 |
✗ Skill "late-command" not found. Available skills: alpha, … — the exact #9821 signature |
✗ hidden skill body injected (HIDDEN SKILL BODY MUST NOT EXECUTE) |
mid e06203e (validation fix only) |
✓ passes | ✗ still injects the hidden body; command executor never called |
head f1defaa1 |
✓ passes | ✓ delegates to the command → COMMAND OUTPUT for dup, display Delegated to command: dup, args forwarded |
The mid arm is the interesting one: the validation fix alone unmasks a quieter, worse failure. Once validation admits the command, loadSkillForRuntime resolves the same-named disable-model-invocation skill and injects its body — silently. On base, the validation bug masked the execution bug (validation rejected before execute ever ran). The second commit fixes exactly what the first unmasked; the bundling is load-bearing, not accidental.
The harness drives the compiled packages/core/dist — real SkillTool, real SkillManager scanning real on-disk skill files — with the two config seams (provider, executor) registered late and no SkillManager change event, mirroring slashCommandProcessor's post-CommandService.create registration. readlink -f confirmed each arm resolved @qwen-code/qwen-code-core into its own tree.
Also scripted on all three arms: late-registered commands appear in the not-found listing (mid/head only); path-gated skills stay gated under a live provider (all arms — the live read's shadowing preserves the gated by paths: branch); provider-throw falls back to the cached snapshot; no-provider (SDK) mode unchanged. Perf: 1000 hit-validations with 2000 registered commands ≈ 100 ms on head vs ≈ 1 ms on base (~0.1 ms/validation); the not-found branch calls the provider twice. Negligible at real command counts.
Mutation matrix — the new tests are not vacuous
Head's test file against source variants (sources swapped via git checkout <oid> -- <files>, restored afterwards):
| source variant | result | red cells |
|---|---|---|
| head (unmutated) | 93/93 green | none — control |
mid e06203e (hidden guard removed) |
6 failed / 87 passed | exactly the 6 new hidden-skill tests |
| base (both fixes removed) | 8 failed / 85 passed | the 2 late-attach tests + the same 6 |
Both fix clusters are pinned by their own tests.
Gates
packages/corevitest (skill.test.ts+skill-utils.test.ts): head 102/102, base 90/90 → +12 passing, +0 failing.tsc --noEmit(core): pass.- Trial merge into current
main(c3d9279932): clean;main's only overlap isconfig.tsin an unrelated region (computer-use removal); affected suite on the merged tree: 102/102.
Findings (all non-blocking)
- The PR body describes only half the diff. "What this PR does" covers the validation live-read but not the execute-layer hidden-skill guard (~45 lines +
hiddenSkillNamesplumbing + 6 tests) that changes behavior when adisable-model-invocationskill shares a name with a model-invocable command. The commit messages do describe it; the PR description should too, since it's user-observable. - [Minor] hidden+disabled wording change:
execute()for a skill that is both hidden and user-disabled now returnsSkill "hd" not found.where base returned… is disabled. Re-enable it via /skills ….validateToolParamsstill says "is disabled", and the scheduler validates first, so this is reachable mainly via direct/SDK invocation. Deliberate ordering consequence; no fix requested. - [Nit] telemetry asymmetry: a command executed via the hidden fallback emits no
SkillLaunchEvent(success)/onSkillLoaded— mirrors the pre-existing disabled-skill path (whose comment justifies it), so/contextattribution skips such commands. Pattern extended, not a regression.
Not covered
A live interactive TUI session end-to-end (the dist harness reproduces the construction/registration ordering that produces the race, but no real TUI was driven); non-interactive CLI ordering was code-read only; eslint/prettier not re-run locally (CI covers).
中文摘要（点击展开）
结论：可合并（merge-ready），58/58 脚本断言全部通过，0 个意外失败。
- 核心声明成立：三臂 A/B（base / 仅校验修复的中间提交 e06203e / head）证明 provider 晚挂载且无 SkillManager 变更事件时，base 以 issue Native slash commands intermittently missing from the Skill-tool surface (async modelInvocableCommands race) #9821 的精确签名拒绝校验（
Skill "late-command" not found. Available skills: …），head 实时读取 provider 后放行。harness 驱动编译后的 dist（真实 SkillTool + 真实 SkillManager + 真实磁盘技能文件），provider/executor 是生产代码自己使用的配置 seam，非 mock。 - 捆绑的第二个修复是必要的且未被 PR 正文描述：中间臂显示，只打校验修复会让模型"安静地"收到 hidden skill 的 body 注入（比 base 的响亮报错更糟）；第二个 commit 修的正是第一个揭开的问题，捆绑合理。建议作者把这一半补进 PR 描述。
- 变异矩阵：head 测试对 base 源码 8 红（2 晚挂载 + 6 hidden）、对中间源码 6 红（仅 hidden）、对 head 93/93 绿——两簇修复都被钉死，非空测试。
- 门禁：head 102/102、base 90/90（+12 通过 / +0 失败）；core
tsc --noEmit通过；与当前 main 试合并无冲突，合并树上 102/102（main 对 config.ts 的改动在无关区域）。 - 次要发现（不阻塞）：hidden+disabled 技能的 execute 措辞从 "is disabled"（带恢复提示）变为 "not found"（深层边缘路径）；hidden 回退执行的命令不计入 SkillLaunchEvent/
/context归因（沿袭既有 disabled 路径的行为）；2000 命令时每次校验 live 读取约 0.1ms——实际数量级下可忽略。 - 未覆盖：真实交互式 TUI 端到端会话（harness 精确复现了竞态时序，但未驱动完整 TUI）；非交互 CLI 注册时序仅代码走读；eslint/prettier 未本地重跑（CI 覆盖）。
- 截图（三臂 A/B、变异矩阵、边缘探针）托管在
pr-assets/9824-verify分支verify/pr9824-local-20260825-133220/，正文已按 raw URL 引用；完整报告与 harness 在本地tmp/pr9824-verify-20260825-133220/。
|
Released in v0.22.2. |
4 participants
What this PR does
SkillTool.validateToolParamsnow consults the model-invocable-commands provider live instead of relying on the command set cached during construction. The live read applies the same file-based-skill name shadowing ascollectAvailableSkillEntries— commands colliding with an active or pending path-activation skill are dropped — so the "gated by paths:" validation branch stays intact. The construction-time cache is kept as the fallback for when no provider is registered (SDK mode) or the provider throws. Adds regression tests and documents late-attach support onsetModelInvocableCommandsProvider.Why it's needed
Fixes the intermittent
Skill "..." not foundfailures for user-level native slash commands reported in #9821. In interactive mode themodelInvocableCommandsProvideris only registered afterCommandService.create(...)resolves (in theslashCommandProcessoreffect), butConfig.initialize()→toolRegistry.warmAll()constructsSkillToolbefore that, so the constructor'srefreshSkills()reads a still-null provider and caches an empty command set. Nothing re-notifies the tool when the provider is attached, so validation keeps rejecting commands for the whole session unless an unrelated SkillManager change event happens to re-runrefreshSkills()— the source of the reported nondeterminism. Meanwhile the per-turn skills drain (drainSkillAndCommandReminders) collects entries fresh and announces the commands as available, so the model invokes a Skill tool that then rejects them. The provider is synchronous, so reading it live on each validation is cheap and makes the check robust to any late attach, not just the interactive CLI's.Reviewer Test Plan
How to verify
Red-before-fix repro: with only the test changes applied (i.e.
git stashof theskill.ts/config.tschanges), runcd packages/core && npx vitest run src/tools/skill.test.ts -t "issue #9821". The two late-attach tests fail with the exact reported signature:With the fix applied the same command passes all 4 regression tests. The tests construct
SkillToolwhile the provider returnsnull(mirroringwarmAll()racingCommandService.create), drain the constructor'srefreshSkills(), then register the provider without firing any SkillManager change event. They cover: late attach passing validation, late-registered commands appearing in the not-found listing, path-gated skills remaining gated under a late provider, and unchanged behavior when no provider is ever registered.Full targeted suites:
cd packages/core && npx vitest run src/tools/skill.test.ts src/tools/skill-utils.test.ts→ 94 passed.npm run typecheck --workspace @qwen-code/qwen-code-core(tsc --noEmit) passes; eslint and prettier clean on the changed files.Evidence (Before & After)
N/A — non-user-visible validation-path change; evidence is the red/green test output in "How to verify".
Tested on
Environment (optional)
Unit tests only (vitest), Node v24.19.0 on Linux.
Risk & Scope
validateToolParamsnow calls the provider on every validation. The provider is a synchronous getter over CommandService's command list, so the cost is negligible; if it throws, validation falls back to the cached snapshot instead of failing.skillManager.notifyConfigChanged()) is deliberately not implemented — validation no longer depends on cache freshness, so it is not required for correctness. Not validated end-to-end in a live interactive TUI session.Linked Issues
Fixes #9821
中文说明
本 PR 做了什么
SkillTool.validateToolParams现在实时读取 model-invocable commands provider，而不再依赖构造时缓存的命令集。实时读取沿用了collectAvailableSkillEntries的同名遮蔽规则——与已激活或待路径激活的文件技能同名的命令会被剔除——因此 "gated by paths:" 校验分支保持不变。构造期缓存仍作为兜底：未注册 provider（SDK 模式）或 provider 抛错时使用。同时补充回归测试，并在setModelInvocableCommandsProvider上注明支持晚挂载。为什么需要
修复 #9821 报告的用户级原生 slash 命令间歇性
Skill "..." not found。交互模式下modelInvocableCommandsProvider要等CommandService.create(...)resolve 后（slashCommandProcessor的 effect 中）才注册，而Config.initialize()→toolRegistry.warmAll()在此之前就已构造SkillTool，构造函数里的refreshSkills()读到 null provider，缓存了空命令集。provider 挂载后没有任何机制重新通知该工具，除非恰好有无关的 SkillManager 变更事件触发refreshSkills()，否则整个会话校验都会拒绝命令——这正是报告中非确定性的来源。与此同时每轮 skills drain（drainSkillAndCommandReminders）实时收集并宣告这些命令可用，模型据此调用 Skill 工具，却被校验拒绝。provider 是同步的，每次校验实时读取开销可忽略，并且对任何晚挂载路径都健壮，而不只是交互 CLI。审阅者测试计划
如何验证
修复前的红测试：只应用测试改动（即
skill.ts/config.ts的改动被git stash时），运行cd packages/core && npx vitest run src/tools/skill.test.ts -t "issue #9821"，两个晚挂载用例会以与报告完全一致的签名失败：应用修复后同一命令 4 个回归用例全部通过。用例构造
SkillTool时 provider 返回null（模拟warmAll()与CommandService.create的竞态），等构造期refreshSkills()落定后再注册 provider，且不触发任何 SkillManager 变更事件。覆盖：晚挂载后校验通过、晚注册命令出现在 not-found 列表、晚挂载 provider 下路径门控技能仍被门控、以及从未注册 provider 时行为不变。完整目标套件：
cd packages/core && npx vitest run src/tools/skill.test.ts src/tools/skill-utils.test.ts→ 94 通过。npm run typecheck --workspace @qwen-code/qwen-code-core（tsc --noEmit）通过；改动文件 eslint 与 prettier 干净。前后证据
N/A——非用户可见的校验路径改动；证据见"如何验证"中的红/绿测试输出。
测试环境
运行环境（可选）
仅单元测试（vitest），Linux，Node v24.19.0。
风险与范围
validateToolParams现在每次校验都会调用 provider。provider 只是对 CommandService 命令列表的同步取值，开销可忽略；若其抛错，校验回退到缓存快照而不是失败。skillManager.notifyConfigChanged()重新通知）刻意未实现——校验已不再依赖缓存新鲜度，正确性上不需要。未在真实交互 TUI 会话中端到端验证。关联 Issue
Fixes #9821