fix(cli): preserve existing skill on reinstall rename failure - #10189
Open
yiliang114 wants to merge 12 commits into
Open
fix(cli): preserve existing skill on reinstall rename failure#10189yiliang114 wants to merge 12 commits into
yiliang114 wants to merge 12 commits into
Conversation
installSkillFromUrl() deleted the existing skill directory before renaming the staged replacement. If the rename failed (e.g. EPERM on Windows), both copies were lost. Use a rollback-capable swap: rename the old directory to a backup sibling first, rename staging in, then remove the backup. On rename failure, restore the backup. Fixes #10187 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
qwen-code-ci-bot
requested changes
Aug 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
@yiliang114 the description isn't using the PR template — it's the free-form Summary / Changes / Test plan format rather than the required sections (What this PR does, Why it's needed, Reviewer Test Plan with How to verify / Evidence (Before & After) / Tested on, Risk & Scope, Linked Issues, and the Chinese translation in <details>).
Two of those sections carry real signal for this particular change: the bug is about rename failures under Windows file/indexer/antivirus locks, so Tested on is the place to record which OSes the reinstall path was actually exercised on, and Risk & Scope is the natural home for the swap's failure edges (e.g. what should happen if restoring the backup itself fails).
Could you repost the description on the template? This gate is about the description only — it makes no judgement on the code.
中文说明
@yiliang114 这个 PR 的描述没有使用 PR 模板——目前是自由格式的 Summary / Changes / Test plan，缺少模板要求的章节（What this PR does、Why it's needed、Reviewer Test Plan（含 How to verify / Evidence (Before & After) / Tested on）、Risk & Scope、Linked Issues，以及 <details> 中的中文翻译）。
对这个改动来说其中两节尤其有价值：这个 bug 正是 Windows 上文件/索引器/杀毒软件锁导致的 rename 失败，Tested on 表格正好说明重装路径实际在哪些系统上验证过；Risk & Scope 也适合说明 swap 的失败边界（比如备份恢复本身失败时的预期行为）。
麻烦按模板重新填写描述。本次门禁只针对描述，不涉及对代码的判断。
— Qwen Code · qwen3.8-max
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — findings reported (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: 70 passed · 0 failed · 70 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：70 通过 · 0 失败 · 70 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10189 — fix(cli): preserve existing skill on reinstall rename failureVerdict: 中文摘要
详见下表：A/B 表（"Central claim"）、mutation 矩阵（"Finding 1"）、H6/H7/H8（"Finding 2"）。 Central claim + A/BCentral claim: a reinstall whose swap rename fails after the old skill directory has been moved aside leaves the previously installed skill intact; on base (delete-then-rename) both copies were lost. Method: the compiled
The A/B verdict is the H1/B1 pair: same fault, head rolls back, base loses the skill. 45/45 harness assertions (assertions.jsonl), all cells green on rerun. FindingsF1 (Suggestion) — the new regression test does not pin the fix it was written forThe PR adds
Both survivors are coverage gaps (behavior is correct — the A/B proves it — but nothing asserts it), not dead code or redundant defence. The failure path the PR exists for (rename fails → backup restored) has zero test coverage; the PR's own test plan checkbox overstates it. A fault-injected test mirroring H1 (stub F2 (Suggestion) — silent
|
qwen-code-ci-bot
requested changes
Aug 26, 2026
…k test Skip `.backup-*` and `.installing-*` entries in both `loadSkillsFromDir` (skill-load.ts) and `SkillManager.loadSkillsFromDir` (skill-manager.ts) so stale transient directories from a crashed reinstall are never loaded as duplicate skills. Add a regression test that verifies the rollback path: when the staging→final rename fails (EPERM), the original skill content is restored and no backup/installing siblings remain. Fixes: review findings on PR #10189 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin and
wenshao
as code owners
August 26, 2026 19:02
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
qwen-code-ci-bot
requested changes
Aug 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
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.
— qwen3.8-max via Qwen Code /review (v0.22.2)
…stall content - Change substring match (.includes) to regex (/\.backup-\d/, /\.installing-\d/) so legitimate skill names like `db.backup-restore` are not incorrectly skipped - Add assertion in reinstall test to verify the new content was actually written Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — findings reported (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: 122 passed · 0 failed · 122 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：122 通过 · 0 失败 · 122 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportSandboxed verification: ❌ not passed — findings reported (agent verdict) — follow-up round at head 中文摘要
详见：A/B 表（"Central claim"）、三臂过滤器表（"Loader filter"）、mutation 矩阵（"Mutation matrix"）。 Previous-round findings status (re-measured at
|
| # | finding (round 1) | severity then | status at new head |
|---|---|---|---|
| F1 | new regression test did not pin the fix (M1/M3 survived) | Suggestion | fixed — commit 2 added restores the original skill when the swap rename fails (EPERM injected via vi.mock('node:fs/promises') rename hook). Re-measured: M1 (swap reverted to delete-then-rename) and M3 (restore removed) now turn exactly that test red, failing at the intended assertion (expect(fs.readFile(skillFile)).resolves.toBe(originalContent) rejects ENOENT, skill-management.test.ts:437), not at setup; the test's own renameCalled assertion proves the fault reaches the code under test; M0 green proves interception works. M2/M4 pins unchanged. |
| F2 | silent .backup-* leftovers discovered by the loader (stale shadowing, reappearance after delete) |
Suggestion | partially fixed — commit 2's loader filter removes the discovery side: leak.head shows a leftover backup sibling is not loaded by either parser, chain.head-after-delete shows the skill no longer reappears after deleting the real dir (base still does). The leak side survives: H7 (EPERM on the backup's removal) still reports install success and leaves a .backup-* sibling; stale backups are still never swept. Downgraded to Nice-to-have (see F6). |
| F3 | description's "pre-existing error in reconnect.ts" did not exist | Correction | re-confirmed — tsc --noEmit exits 0 with zero diagnostics in both packages/cli and packages/core at the new head. |
Central claim + A/B (re-run at new head)
Central claim: a reinstall whose swap rename fails leaves the previously installed skill intact; on base (delete-then-rename) both copies were lost. Re-proven at 35a9646e with the same method as round 1: compiled dist installManagedSkill() per cell, QWEN_HOME seam, fixture stub at the network boundary, LD_PRELOAD rename(2)/unlinkat(2) fault shim (code under test unmodified). Base arm = esbuild-transpiled HEAD^1 copy of the single changed cli file; the install path's only core import is Storage (storage.ts diff vs base is empty), so the new core filter cannot confound the install A/B. Witnesses: 01-ab-head-rollback-on-eperm.png, 02-ab-base-data-loss.png.
| cell | arm | fault injected | oracle | result |
|---|---|---|---|---|
| H1 | head | EPERM on staging→skillDir |
rejects EPERM; pptx + OLD-BODY intact; no leftovers; trace shows backup-rename ok, swap EPERM, restore ok |
8/8 PASS |
| B1 | base | identical | rejects EPERM; entries [] — old skill gone (data loss); no leftovers |
5/5 PASS (control fails as predicted) |
| H2/B2 | head/base | none | reinstall succeeds, NEW-BODY installed, orphan orphan-old.md dropped, no .backup-*/.installing-* leftovers, refreshCache ×1 |
7/7 each (parity) |
| H3/B3 | head/base | none | fresh install succeeds (ENOENT on backup rename swallowed) | 3/3 each (parity) |
| H4 | head | EPERM on swap AND restore | rejects EPERM; skill dir absent; exactly one .backup-* holding OLD-BODY (recoverable); no staging leftovers |
5/5 |
| H5 | head | EPERM on first rename | rejects EPERM; old dir untouched; no leftovers | 5/5 |
| H7 | head | EPERM on backup removal | install reports success (NEW-BODY) and leaves one .backup-* sibling (leak survives, defused — see F6) |
4/4 |
The H1/B1 pair is the verdict: same fault, head rolls back, base loses the skill. 47/47 cell assertions.
Loader filter (new surface, commits 2–3)
The PR adds the same regex filter to both parsers — loadSkillsFromDir (skill-load.ts, extension skills) and SkillManager.loadSkillsFromDir (skill-manager.ts, bundled/project/user). Both were driven from the compiled dist (head), the git-base source bundled with esbuild (base), and a head-with-regex-reverted-to-.includes variant (the intermediate commit's shape, per its message). Corpus: 8 directory names, each with a valid SKILL.md whose frontmatter name is legal under SKILL_NAME_PATTERN (/^[\p{L}\p{N}_:.-]+$/u — dots are legal skill names). Witness: 03-filter-three-arms.png.
| corpus entry | base (no filter) | substring (commit-2 shape) | head (regex) |
|---|---|---|---|
pptx |
load | load | load |
pptx.backup-13000-1756000000000 (leftover) |
load (old bug) | skip | skip (fixed) |
pptx.installing-13000-1756000000000 (staging) |
load (old bug) | skip | skip (fixed) |
db.backup-restore (legit, PR's example) |
load | skip (regression) | load (commit 3 load-bearing) |
db.installing-restore (legit) |
load | skip (regression) | load |
backup-123 (legit, no dot) |
load | load | load |
db.backup-2024 (legit, single digit group) |
load | skip | skip (F5 false positive) |
db.backup-2024-1756000123456 (artifact-shaped legit) |
load | skip | skip (unavoidable collision) |
58/58 filter assertions, including: both parsers make identical decisions on every arm (the duplicated predicate agrees — identity.head/base/substr); a real leaked-dir shape is discovered only by base (leak.*); after deleting the real skill, head loads 0 skills while base resurrects the backup (chain.*); and the measured stricter candidate \.backup-\d+-\d+$ (plus installing twin) still skips every artifact shape while keeping db.backup-2024 (strictfix.*).
Mutation matrix at 35a9646e
Witness: 04-mutation-matrix.png. CLI suite = 8 tests; core skills suites = 152 tests (run with QWEN_HOME= cleared, see Not covered). Every mutant restored byte-identical (sha-verified; final git status clean).
| mutation | suite | result | classification |
|---|---|---|---|
| M0 unmutated (control) | cli | 8/8 green | harness live |
| M1 swap reverted to base delete-then-rename | cli | RED — rollback test fails at intended assertion | killed (was survivor in round 1 → F1 fixed) |
| M2 remove post-success backup cleanup | cli | RED — cleans up the backup directory… |
killed (pin unchanged) |
| M3 remove rollback restore in catch | cli | RED — rollback test fails | killed (was survivor in round 1 → F1 fixed) |
| M4 rethrow ENOENT on backup rename | cli | RED — fresh-install test | killed (positive control) |
| C0 unmutated (control) | core | 152/152 green | harness live |
| C1 remove filter (skill-load.ts) | core | green | survivor → coverage gap |
| C2 remove filter (skill-manager.ts) | core | green | survivor → coverage gap |
| C12 remove BOTH filters (combination) | core | green | confirms the set is unpinned, not layered-defence |
C3 regex → .includes (both) |
core | green | survivor → commit-3 claim unpinned |
| R1 candidate fix: strict regex (both) | core | green | suite pins nothing along this axis (name the fixture, F4) |
CP1 SKILL_MANIFEST_FILE → 'SKILLx.md' |
core | green | invalid control, dropped: skill-load.test.ts mocks fs/promises wholesale, so the manifest filename is unobservable there (pre-existing suite property) |
| CP1B break frontmatter regex in parseSkillContent | core | RED — 23 tests | positive control (same file) |
CP2 loadSkillsFromDir → return [] |
core | RED — 26 tests | positive control (same file) |
Findings
F4 (Suggestion) — the new loader filter has no test coverage
Removing either filter, both, or reverting the regex to the substring form leaves all 152 core skills tests green (C1/C2/C12/C3), while CP1B/CP2 prove the suites do exercise both files. The behavior is correct — the three-arm filter table above is the proof — but nothing in the repo pins it: a future edit that re-breaks db.backup-restore loading or re-admits .backup-* leftovers would ship green. The round-1 F1 lesson applied to a new surface. A fixture costs two readdir entries per parser's existing loadSkillsFromDir describe (one named pptx.backup-123-456, one db.backup-restore); note skill-load.test.ts would need the mocked fs.readdir to return those entry names, since its fs layer is mocked.
F5 (Nice-to-have) — the filter regex false-positives on legit names with a single digit group
/\.backup-\d/ is unanchored and needs only one digit, so a legitimate skill directory db.backup-2024 (legal name, legal dir) is silently never loaded by either parser (load.head.db.backup-2024). Real artifacts are <slug>.backup-<pid>-<ts> — always two dash-separated digit groups — so the measured stricter /\.backup-\d+-\d+$/ (and installing twin) skips every artifact shape in the corpus and keeps db.backup-2024 (strictfix.* assertions). The residual collision (db.backup-2024-1756000123456, artifact-shaped) is unavoidable and acceptable; the avoidable half is this one. Suggested fix (measured in the harness, not applied to the PR): anchor to the two-group suffix.
F6 (Nice-to-have) — post-success backup cleanup failure is still silent (leak survives, defused)
H7: EPERM on the backup's removal → install returns installed: true and leaves a pptx.backup-* sibling; the .catch(() => {}) leaves no log, field, or reader, so the failure is unobservable even in devtools, and stale backups from crashed installs are still never swept. Round 1's sharpest consequences are gone — the leftover is no longer loaded by either parser and no longer resurrects a deleted skill — so what remains is a bounded disk leak plus an unobserved failure, not stale shadowing. Surfacing the cleanup failure (or sweeping stale *.backup-* siblings of the target slug on install) would close it.
Not covered
- Real Windows lock semantics: the shim reproduces the EPERM error code and rename-failure shape at the syscall level, not the OS-level cause.
- Concurrent reinstalls of the same slug; staging write-failure path (byte-identical pre-existing code on both arms); real
downloadSkillnetwork path (intercepted at the network boundary; sandbox has no network). - Repo-wide test/lint/typecheck gates. Gates run: cli
skill-management.test.ts8/8; core skills trio 152/152;tsc --noEmitin cli and core (zero diagnostics); eslint on the four changed files (clean; liveness proven by a plantedno-explicit-anyviolation that was caught — an unused-var plant is ignored by design,varsIgnorePattern: '^_'). - Environment caveat: the four
skill-manager.test.tsfailures observed initially (should return all user-level base dirsetc.) are caused by this container'sQWEN_HOMEenv var overriding the mockedos.homedir()inStorage.getGlobalQwenDir()— withQWEN_HOME=cleared the suite is 152/152 at head. Not PR-caused; real CI (noQWEN_HOME) is unaffected. - Per-commit attribution:
git rev-list HEAD^1..HEAD^2= 1 locally vs 3 commits in the metadata (shallow depth-2 checkout) — only the aggregateHEAD^1..HEADdiff was exercised; the substring variant represents the intermediate commit per its message, not its bytes.
Methodology
Environment: CI verify container, refs/pull/10189/merge checkout (depth 2), pre-built at head. The A/B harness imports the compiled dist skill-management.js (head) and an esbuild-transpiled HEAD^1 copy (base) with a fixture stub for the network-boundary module and an LD_PRELOAD shim (fault.c, gcc-built) faulting rename(2)/renameat(2)/renameat2(2)/unlink/unlinkat/rmdir per substring rules, logging every intercepted syscall. The loader harness drives both real parsers from compiled dist (head) and esbuild-bundled git-base / patched copies (base/substring), with variant equivalence proven byte-exact ("head minus the additive filter block == git base"). Mutations applied to scratch copies of the source, suites run via each package's vitest, sources restored byte-identical (sha-verified, git status clean). Raw logs, harness scripts, shim source, and tally live in this artifact dir; counts: 122 pass / 0 fail / 122 total.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/cli/src/acp-integration/skill-management.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/skill-management.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/skill-management.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/cli/src/acp-integration/skill-management.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/skill-management.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/skill-management.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/skill-management.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/skill-management.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
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 26, 2026
The restoreLoadedSkillsFromHistory method added to client.ts calls this.config.getToolRegistry() but the minimal config mock in client.telemetrySwap.test.ts did not provide it, causing 9 test failures with "TypeError: this.config.getToolRegistry is not a function". Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: Skipped because GitHub had not computed the PR merge ref after several retries — try again shortly. 中文 — 判定：
|
Collaborator
|
Eighth gate pass — head moved again. Two new commits since the round-seven approve on
Moving on to code review of the two new commits. 🔍 中文说明第八次门禁 —— head 再次移动。自第七轮在
进入对两个新 commit 的代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewEighth pass — head Round-3 finding R2-1 (the anchored filter could still hide legitimate skill names) — resolved. The fix reserves the artifact shape at validation instead of widening the filter: R3-1 / R3-2 (no fixture pins the The swap fix itself is untouched by the two new commits and stands as proven in earlier rounds — byte-exact rollback asserted by the EPERM regression test, and A/B-proven in the sandboxed verify at Non-blocking, carried forward from earlier rounds: the artifact predicate is still hand-duplicated across Test evidence — the PR's own CI (checks API; nothing re-run here)CI on
中文说明第八轮代码审查（针对 第三轮发现 R2-1（锚定过滤器仍可能隐藏合法技能名）—— 已解决。 修复方式是在校验层保留产物形态，而非放宽过滤器：托管与 workspace 两个验证器都拒绝匹配 R3-1 / R3-2（无 fixture 钉住 交换修复本体未被这两个 commit 触动，维持此前各轮结论 —— EPERM 回归测试断言逐字节回滚，且已在 非阻塞、沿用此前各轮：产物判定仍手写重复于两个加载器（作者已声明后续跟进）；清理失败仍被 测试证据： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — every blocking finding across eight rounds is resolved and statically verified at the new head, CI is settled green end to end; what remains is named non-blocking hygiene (drop the redundant telemetrySwap hunk on rebase; predicate dedup, cleanup-failure logging, and artifact reclamation are follow-up material). Honest read, eighth pass. My independent answer to "the filter can still hide a legitimate name" was one of two things: park the artifacts outside the scanned root, or reserve the shape where names are validated. The author took the second, smaller path, and it closes the loop properly — the reservation regex is the exact union of the loader filters, both install entry points run it before touching disk or source, and the charset leaves no anchor-smuggling gap. The two new fixtures pin the Two honest caveats. First, the residual recorded in Stage 2: a hand-placed directory named exactly like an artifact ( For context: the standing Verdict: approve now — CI is settled green on 中文说明置信度：4/5 —— 八轮以来所有阻塞性发现均已解决并在新 head 上经静态核验，CI 已全绿稳定；余下均为点名的非阻塞卫生问题（rebase 时去掉冗余的 telemetrySwap 行；判定去重、清理失败日志、产物回收留作后续跟进）。 第八轮实读。对"过滤器仍可能隐藏合法名字"，我的独立答案是两条路之一：把产物挪出扫描根目录，或在名字校验处保留该形态。作者选了更小的第二条，且闭环完整：保留正则与加载器过滤器的并集完全一致，两个安装入口都在触碰磁盘或来源之前执行，字符集封死了锚绕过。两个新 fixture 以误伤对照钉住 两点如实说明的保留。其一，Stage 2 记录的残留：手工放置、名字恰似产物（如 背景说明：PR 上当前挂着的 裁决：立即批准 —— — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
requested changes
Aug 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
The swap fix itself is proven and right — the regression test pins it and the sandboxed A/B shows base losing the skill under the same injected EPERM while this branch rolls back. Holding on the loader-filter increment, reviewed at 7ca706643ea9ade29de601ed37e68aa9fb2958a0:
- Unanchored artifact regex — measured false positive.
/\.backup-\d///\.installing-\d/match anywhere in the directory name, so a legitimate skill dir likedb.backup-2024installs successfully but is silently never loaded by either loader — the same silent-failure class this PR exists to eliminate. The anchored/\.backup-\d+-\d+$/shape was measured at the previous head: it preservesdb.backup-2024and still skips every real artifact (<slug>.backup-<pid>-<ts>). One line per file (packages/core/src/skills/skill-load.ts,packages/core/src/skills/skill-manager.ts). - Neither filter block is pinned by a test. The suite stays green with both blocks deleted (measured by the sandboxed run:
skill-management.test.ts8/8, core suites byte-identical). One test per loader — seed a real skill plus.backup-<pid>-<ts>/.installing-<pid>-<ts>siblings holding validSKILL.mdfiles, assert only the real skill loads, and confirm each goes red when its filter is removed.
Full reasoning in the Stage 2/3 comments above. Non-blocking notes there too: the duplicated filter block could become one shared predicate, the swallowed backup-cleanup failures deserve a follow-up issue, and the description still needs the PR template (Tested on matters for a Windows-lock-driven bug) plus the reconnect.ts typecheck claim struck — tsc --noEmit is clean on both packages.
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 27, 2026
The reinstall-artifact filter used unanchored regexes (/\.backup-\d/ and /\.installing-\d/), which matched anywhere in a directory name, so a legitimate skill dir like `db.backup-2024` installed fine but was silently never loaded. Anchor both patterns to the actual artifact shape (`<slug>.backup-<pid>-<timestamp>` / `<slug>.installing-<pid>-<timestamp>`) in both loaders (skill-load.ts and skill-manager.ts). Add skill-install-artifacts.test.ts pinning each loader's filter block: seeds a real skill plus `.backup-<pid>-<ts>` / `.installing-<pid>-<ts>` siblings holding valid SKILL.md files and a legit `db.backup-2024` dir, asserting only the real and legit dirs load. Verified red when either filter is removed or reverted to the unanchored form. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定：❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定：❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
requested changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-3 stale
.backup-*/.installing-*directories still never reclaimed from the global skills dir — already reported (comment 3866632519), author declined with a follow-up issue planned - R2-4 artifact predicate still hand-duplicated across the two loaders — already reported (comment 3866632522), author declined as out of scope for this bugfix
Convergence: round 3 posted 3 inline comment(s), 3 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/skills/skill-load.ts (findings in round 2; 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.)
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛情况：第 3 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/core/src/skills/skill-load.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
The anchored loader filters skip directories shaped exactly like the reinstall swap artifacts (`<slug>.backup-<pid>-<timestamp>` / `.installing-<pid>-<timestamp>`). A skill whose slug/name legitimately ends in that shape installed successfully yet was silently skipped by both loaders, so it never appeared in listings and could never be invoked while every reinstall kept reporting success. Reject such slugs in validateSkillSlug and such names in validateWorkspaceSkillName with a clear error, so the loader filters have no API-installable name left to eat. Names merely containing the markers (e.g. db.backup-restore) remain valid. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Seed two more legitimate skill dirs into the loader-filter tests: `db.backup-2024-1234-archive` (name continues after the two digit groups, so dropping the `$` end anchor filters it and both tests go red) and `data-backup-2024-06` (hyphen instead of a dot before `backup-`, so dropping the `\.` requirement filters it and both tests go red). Both mutations were confirmed to turn the suite red before restoring the production regexes. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
🩺 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 |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定：❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
qwen-code-ci-bot
previously approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- artifact predicate hand-duplicated across loaders and validators — already reported as R2-4 (comment 3866632522); author declined dedup as out of scope for this bugfix
- stale
.backup-*/.installing-*sweep in installSkillFromUrl — already reported as R2-3 (comment 3866632519); author declined with a follow-up issue planned - swallowed restore/cleanup
.catch(() => {})failures in the swap block — already discussed in the R2-3 thread (comment 3866632519) and the round-3 review body; classified as follow-up material
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/skill-management.ts:319 — [review] Reinstall swap protocol duplicated across installerspackages/cli/src/acp-integration/skill-management.ts:329 — [probe] Non-ENOENT backup-rename rethrow guard untestedpackages/cli/src/acp-integration/skill-management.ts:323 — [probe] Artifact name generator unpinned against filter shape
Convergence: round 4 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 3 (3 new). The rate of new findings is not falling. 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.)
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
Collaborator
Local verification report — real environment, A/B against
|
| Result | |
|---|---|
The bug in #10187 reproduces on main |
✅ confirmed |
| This PR fixes it | ✅ confirmed |
| The two new name guards work end to end | ✅ confirmed |
| New tests actually hold the behavior down | ✅ 7/7 mutants killed |
| Existing suites | ✅ no regression (core 22 271 pass; cli's 14 failures are pre-existing / load flakes) |
| Merging as-is | ❌ the merged tree does not compile — one-line fix, see E5 |
| Residual gaps |
E1 — the fix works (headline)
Reinstall a skill that is already installed; the staging → final rename fails with EPERM.
main:~/.qwen/skills/ends up empty. The 57-file skill, including files the user added, is destroyed.- This PR: install fails with the same
EPERM,SKILL.mdis byte-identical, all 57 files and the user's ownUSER-NOTES.mdsurvive, and no.backup-*/.installing-*sibling is left behind.
E2 — ⚠️ the crash window is still open, and now leaks twice as much
Same setup, but the interposer calls _exit(137) at the swap point instead of returning EPERM (power loss / OOM kill / SIGKILL).
main: leaves one orphan (pptx.installing-*); the skill is still listed, but pointing into the staging directory.- This PR: leaves two orphans (
pptx.backup-*andpptx.installing-*), and because the loaders now filter both shapes, the skill silently disappears from the product even though a complete copy is sitting right next to it. - Neither build ever sweeps them. A subsequent successful reinstall re-creates
pptxand leaves both orphans in place — 1.2 MB → 3.6 MB after a single crash, growing with every crash.
The sibling installer in packages/cli/src/serve/workspace-skill-management.ts already solves exactly this: it calls removeInstallArtifacts() before staging, and keeps its staging/backup directories in path.dirname(baseDir) (.qwen/) instead of inside .qwen/skills/.
E3 — ⚠️ the loader filter also drops a legitimate skill
The filter correctly hides a crashed-reinstall staging directory (a real pre-existing bug on main, visible in the same screenshot). But SKILL_NAME_PATTERN is /^[\p{L}\p{N}_:.-]+$/u, so db.backup-2024-06 is a perfectly legal skill name — and it now silently stops loading. No error, no warning; only a debugLogger.debug line.
The PR's own tests pin db.backup-2024 (one digit group) and data-backup-2024-06 (hyphen, not dot). Adding one more date component to the first case flips it, and that case is not covered.
E4 — the new tests have real discriminating power
Each mutant reverts exactly one production hunk on this branch. All seven are killed, including the two that check the regexes' $ anchor and literal dot.
Full suites on the merged tree: packages/core 22 271 passed / 0 failed. packages/cli 26 018 passed / 14 failed — 12 are timeout flakes under full-suite load that pass on re-run (AuthDialog, serve/server), and the 2 SystemController > get_usage_info failures reproduce identically on a clean origin/main checkout. None touch skills.
E5 — ❌ blocking: the merge is clean but does not compile
git merge-tree origin/main pr10189 reports no conflicts and GitHub says MERGEABLE, but main has since landed the same getToolRegistry mock line in packages/core/src/core/client.telemetrySwap.test.ts. Git text-merges both copies into one object literal:
src/core/client.telemetrySwap.test.ts(101,5): error TS1117: An object literal cannot have multiple properties with the same name.
tsc --build fails, so npm run build fails and nothing downstream of packages/core builds. Fix: drop this PR's client.telemetrySwap.test.ts hunk entirely — origin/main already carries it. (Everything else in this report was measured with that one line removed.)
E6 — the guards work; note the migration cost
- Deleting a skill while a stale
.backup-*sibling exists: onmainthe deleted skill comes back from the backup; with this PR it stays buried. ✅ POST /workspace/skills/installwithname: "foo.backup-1-2":maininstalls it happily; this PR rejects it up front withinvalid_skill_name. Normal installs are unaffected. ✅- But a workspace that already has
foo.backup-1-2installed (installed by the product itself, on an older build) silently loses it after upgrading.
Recommendation
The core change is correct and well tested — E1 is exactly the fix #10187 asked for, and I could not break it. Before merging:
- Required — drop the
packages/core/src/core/client.telemetrySwap.test.tshunk (E5). Without it the merge breaks the build for everyone. - Worth considering — put the staging and backup directories in
path.dirname(skillsBaseDir)(~/.qwen/) instead of inside~/.qwen/skills/, and sweep leftovers before staging, exactly asinstallWorkspaceSkillalready does. The skill loaders scan<root>/<provider-dir>/skills(plus any explicitly configured custom skill dirs), so artifacts one level up are invisible by construction: no loader filter in two files, no reserved suffix in two validators, no false positive (E3), no silent-migration cost (E6), and the crash leak (E2) gets cleaned up. That would shrink this PR rather than grow it. - If the current shape is kept — at minimum add the artifact sweep from E2, and consider raising the "skipping install artifact entry" log above
debugso a silently dropped skill is diagnosable.
Reproduction details
- macOS (Darwin 25.6.0, arm64), Node v24.18.1.
- Tree: PR head
a32986d20dmerged withorigin/maind3ab6ea98a(merge commit79be6f52ff),npm run build && npm run bundle. - "before" build: same tree,
git checkout origin/main --onskill-management.ts,workspace-skill-management.ts,skill-load.ts,skill-manager.ts, rebuilt and re-bundled. Verified the two bundles differ only in the expected code. - Driver: newline-delimited JSON-RPC over stdio to
dist/cli.js --experimental-acp—initialize, thenqwen/skills/install/qwen/skills/delete. Listings viaGET /workspace/skillsondist/cli.js serve. - Fault injection: a ~40-line C dylib loaded with
DYLD_INSERT_LIBRARIESthat interposesrename/renameatand returnsEPERM(or calls_exit(137)) when the source contains.installing-and the destination ends with/skills/pptx. Every other rename in the process passes through and is logged. - Source URL used throughout:
https://github.com/anthropics/skills/blob/main/skills/pptx/SKILL.md(real network download).
中文说明
本地真实环境验证报告（与 main 做 A/B）
我把本分支与当前 main 合并后完整构建，用真实打包产物（dist/cli.js --experimental-acp 与 dist/cli.js serve）在隔离的 QWEN_HOME 里跑真实的 anthropics/skills 下载（pptx skill，57 个文件 / 1.2 MB）。EPERM 不是 mock 出来的：用 DYLD_INSERT_LIBRARIES 注入的拦截库在真实的 rename(2)/renameat(2) 系统调用上返回失败，生产代码原样运行。「修改前」那一侧是同一棵树，只把 4 个生产文件切回 origin/main 后重新构建、重新打包——所以下面每一处差异都可归因于本 PR。
| 结论 | |
|---|---|
#10187 的问题在 main 上能复现 |
✅ 确认 |
| 本 PR 修好了 | ✅ 确认 |
| 两处新增的名字校验端到端有效 | ✅ 确认 |
| 新增测试确实兜住了新行为 | ✅ 7/7 变异体被杀 |
| 既有测试套件 | ✅ 无回归（core 22271 通过；cli 的 14 条失败是既有问题/负载抖动） |
| 按现状合入 | ❌ 合并后的树编译不过 —— 一行即可修，见 E5 |
| 遗留缺口 |
E1 — 修复有效（核心结论）
对一个已安装的 skill 重装，让暂存目录 → 正式目录的 rename 以 EPERM 失败：
main：~/.qwen/skills/最后是空的，57 个文件（含用户自己加的文件）全部被销毁。- 本 PR：安装以同样的
EPERM失败，但SKILL.md逐字节一致，57 个文件与用户的USER-NOTES.md都在，也没有残留.backup-*/.installing-*兄弟目录。
E2 — ⚠️ 崩溃窗口仍然存在，而且泄漏量翻倍
同样的环境，把拦截库改成在交换点直接 _exit(137)（断电 / OOM kill / SIGKILL）：
main：残留 1 个孤儿目录（pptx.installing-*）；skill 仍然被列出，但指向的是暂存目录。- 本 PR：残留 2 个孤儿目录（
pptx.backup-*和pptx.installing-*），而且因为 loader 现在把这两种形状都过滤掉，skill 在产品里直接消失——尽管旁边就躺着一份完整副本。 - 两侧都不会清扫。之后一次成功的重装会重建
pptx，但两个孤儿目录原封不动：一次崩溃就从 1.2 MB 涨到 3.6 MB，每崩一次涨一次。
同仓库里的兄弟实现 packages/cli/src/serve/workspace-skill-management.ts 早就解决了这件事：它在暂存前调用 removeInstallArtifacts()，并且把暂存/备份目录放在 path.dirname(baseDir)（即 .qwen/）而不是 .qwen/skills/ 里面。
E3 — ⚠️ 过滤器也会误伤合法 skill
过滤器确实挡住了崩溃残留的暂存目录（这是 main 上真实存在的既有 bug，同一张图里可见）。但 SKILL_NAME_PATTERN 是 /^[\p{L}\p{N}_:.-]+$/u，db.backup-2024-06 是完全合法的 skill 名字——它现在会静默地不再加载：没有报错、没有警告，只有一条 debugLogger.debug。
PR 自己的测试钉了 db.backup-2024（只有一组数字）和 data-backup-2024-06（是连字符不是点）。给第一个再加一段日期就翻车了，而这个用例没有覆盖。
E4 — 新测试有真实的辨别力
每个变异体只回退一个生产 hunk。7 个全部被杀，包括专门验正则 $ 锚点和字面点的那两个。
合并树上的完整套件：packages/core 22271 通过 / 0 失败。packages/cli 26018 通过 / 14 失败——其中 12 条是满负载下的超时抖动，单独重跑即过（AuthDialog、serve/server），另外 2 条 SystemController > get_usage_info 在干净的 origin/main 上以同样方式失败。都与 skills 无关。
E5 — ❌ 阻塞项：合并无冲突，但编译不过
git merge-tree origin/main pr10189 没有冲突，GitHub 也显示 MERGEABLE，但 main 之后已经合入了同一行 getToolRegistry mock（packages/core/src/core/client.telemetrySwap.test.ts）。git 把两份文本都并进了同一个对象字面量：
src/core/client.telemetrySwap.test.ts(101,5): error TS1117: An object literal cannot have multiple properties with the same name.
tsc --build 失败 → npm run build 失败 → packages/core 下游全部构建不出来。修法：把本 PR 里 client.telemetrySwap.test.ts 的改动整块删掉——origin/main 已经有了。（本报告其余所有测量都是在删掉这一行之后做的。）
E6 — 校验有效；注意迁移成本
- 在存在陈旧
.backup-*兄弟目录时删除 skill：main上被删的 skill 会从备份里复活，本 PR 则保持隐藏。✅ POST /workspace/skills/install传name: "foo.backup-1-2"：main会照装不误，本 PR 在读取来源之前就以invalid_skill_name拒绝。正常安装不受影响。✅- 但已经装了
foo.backup-1-2的工作区（而且是产品自己在旧版本上装的），升级后会静默丢失这个 skill。
建议
核心改动是正确的，测试也扎实——E1 正是 #10187 要的修复，我没能把它打破。合入前：
- 必须——删掉
packages/core/src/core/client.telemetrySwap.test.ts的改动（E5）。否则合入即破坏所有人的构建。 - 建议考虑——把暂存目录和备份目录放到
path.dirname(skillsBaseDir)（即~/.qwen/）而不是~/.qwen/skills/里面，并在暂存前清扫残留，跟installWorkspaceSkill现在的做法完全一致。skill loader 只扫描<root>/<provider-dir>/skills（以及显式配置的自定义 skill 目录），因此上一层的产物在构造上就不可见：两个文件里的 loader 过滤没了，两个校验器里的保留后缀没了，E3 的误伤没了，E6 的静默迁移成本没了，E2 的崩溃泄漏也顺带清掉。这个方向会让 PR 变小而不是变大。 - 若维持现有形状——至少补上 E2 的残留清扫，并考虑把 "skipping install artifact entry" 的日志级别提到
debug以上，让被静默丢弃的 skill 可诊断。
复现细节
- macOS（Darwin 25.6.0, arm64），Node v24.18.1。
- 代码树：PR head
a32986d20d与origin/maind3ab6ea98a合并（合并提交79be6f52ff），npm run build && npm run bundle。 - 「修改前」构建：同一棵树，对
skill-management.ts、workspace-skill-management.ts、skill-load.ts、skill-manager.ts执行git checkout origin/main --后重新构建打包；已核对两份 bundle 只在预期代码处不同。 - 驱动方式：按行分隔的 JSON-RPC 走 stdio 打
dist/cli.js --experimental-acp——先initialize，再qwen/skills/install/qwen/skills/delete；列表用dist/cli.js serve的GET /workspace/skills。 - 故障注入：约 40 行的 C 动态库，用
DYLD_INSERT_LIBRARIES拦截rename/renameat，当源路径含.installing-且目标路径以/skills/pptx结尾时返回EPERM（或调_exit(137)）。进程内其余所有 rename 一律放行并记账。 - 全程使用的来源 URL：
https://github.com/anthropics/skills/blob/main/skills/pptx/SKILL.md（真实网络下载）。
origin/main already carries the same getToolRegistry mock (plus later LlmClient rename), so merging both copies yields a duplicate object literal property (TS1117) that breaks the build. Align this file with main as suggested in the local verification report (E5). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…-reinstall-atomic
Collaborator
Author
|
Thanks for the thorough A/B verification. E5 fixed: E2 (crash-window orphan sweep) and E3 (loader filter false positive) are noted as optional follow-ups, not addressed in this PR. |
Collaborator
Author
|
Closeout: reviewed the three remaining suggestions at current head. I am not moving the reserved install-artifact suffix check automatically because that changes compatibility/API behavior for legacy skills whose names already match the reserved shape. Recommended human decision: keep install-time rejection, but decide whether delete/toggle should bypass the reserved-suffix rule for pre-existing legacy directories. No code changes from this sweep. |
Keep reserved install-artifact suffix rejection on install paths while allowing management paths to delete or toggle legacy skills whose names already match that shape. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Closeout: moved the reserved install-artifact suffix rejection to install-only paths, so new installs still fail loudly for loader-skipped names while delete/toggle can clean up legacy artifact-shaped skills. Added ACP and workspace coverage for legacy cleanup, near-miss accepted names, and pre-source install rejection. Verified locally before the base refresh: |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Aug 29, 2026
Collaborator
Re-verification of head
|
| Previous finding | Status on d85affc816 |
|---|---|
| E5 merged tree fails to compile (blocking) | ✅ fixed — npm run build exits 0 |
| E1 the fix itself | ✅ still holds |
| E4 test suite discriminating power | ✅ 7/7 mutants killed on the new code shape |
| E6 legacy artifact-shaped skills become unmanageable | |
| E2 crash window leaks two orphans, never swept | ❌ unchanged |
E3 legit db.backup-2024-06 silently dropped |
❌ unchanged |
E7 — e0f831ed27 works, but only on the ACP path
Moving the reserved-suffix check out of validateSkillSlug / validateWorkspaceSkillName and into install-only helpers does what it says for qwen/skills/*: a stranded $QWEN_HOME/skills/foo.backup-1-2/ can be toggled and deleted again, while install is still refused.
Note the third row: setEnabled at project scope still fails with Project skill not found, because that path resolves through the loader-backed listing instead of reading the directory directly.
E8 — ⚠️ through qwen serve (the surface the Web Shell uses) the skill is still unreachable, at both scopes
DELETE /workspace/skills/:name no longer trips the name guard — it now fails one step later instead:
main |
PR, previous head | PR, d85affc816 |
|
|---|---|---|---|
| listed at all | yes | no | no |
DELETE …?scope=workspace |
deleted: true |
invalid_skill_name |
skill_not_found |
DELETE …?scope=global |
deleted: true |
— | skill_not_found |
| left on disk | — | yes | yes |
The cause is WorkspaceService.deleteWorkspaceSkill() (packages/cli/src/serve/workspace-service/index.ts:1048): it looks the skill up in getWorkspaceSkillsStatus() to obtain installedPath, and throws WorkspaceSkillNotFoundError when it isn't there. The loader filter empties that listing for exactly these names, so the reachable route can never resolve one. Relative to main this is a behaviour regression at the HTTP surface — and it is the very case the new commit set out to unblock.
The new test does not catch it because it calls the module-level function with an explicit path:
// packages/cli/src/serve/workspace-skill-management.test.ts
await expect(
deleteWorkspaceSkill(workspace, 'workspace', name, skillFile), // installedPath handed in
).resolves.toEqual({ skillName: name, scope: 'workspace', deleted: true });The route never has skillFile; it asks the service to find it. A test that goes through WorkspaceService.deleteWorkspaceSkill(ctx, name, scope) — or an integration test against the route — would have failed.
Also worth noting: because such a skill is listed nowhere, a user has no way to discover the name to pass to the ACP method that does still work.
E2 / E3 — unchanged
Re-ran both on the new head, same results: a hard kill at the swap still leaves pptx.backup-* and pptx.installing-*, the skill still vanishes from the product while a complete copy sits next to it, a later successful reinstall still sweeps neither (1.2 MB → 3.6 MB), and db.backup-2024-06 is still silently dropped.
E4 — tests and suites
7/7 mutants killed against the new code shape (the two guards are now separate rejectInstallArtifact* helpers, so M2/M5 revert the call sites). packages/core 22 328 passed / 0 failed. packages/cli 26 048 passed / 13 failed — 11 AuthDialog timeouts that pass 25/25 on a serial re-run, plus the 2 SystemController > get_usage_info failures that reproduce on a clean origin/main.
Where this leaves the PR
The blocking item is gone and the core fix is solid, so this is mergeable now if you want it in. What is left is that each patch has closed one entrance while the next one stays open — E6 → E7 fixed ACP, E8 opened; and E2/E3 are still there. Two ways forward:
- Minimal, keeps the current shape — give
WorkspaceService.deleteWorkspaceSkilla direct-path fallback when the listing lookup misses (mirror thescope === 'global'branch ofreadManagedSkillFile, which is exactly why the ACP path works), test it through the service rather than the module function, and add the artifact sweep from E2. - Structural, and smaller overall — put staging and backup in
path.dirname(skillsBaseDir)(~/.qwen/) likeinstallWorkspaceSkillalready does, and sweep leftovers before staging. The loaders only scan<root>/<provider-dir>/skills(plus configured custom dirs), so nothing needs to learn the artifact shape: the two loader filters, both reserved-suffix guards, the install-only/management-path split, E3's false positive and E8's stranding all stop existing, and E2's leak gets cleaned up.
中文说明
对新 head d85affc816 的复验
方法与上轮相同：真实打包产物、真实 GitHub 下载、隔离 QWEN_HOME、用 DYLD_INSERT_LIBRARIES 在真实 rename(2) 系统调用上注入 EPERM，A/B 的「修改前」一侧是同一棵树只把 4 个生产文件切回 origin/main。新 head 已经包含 origin/main（跑测试时为 c1f8a422fc），所以 head 本身就是合并结果，无需再合。
| 上轮的发现 | 在 d85affc816 上的状态 |
|---|---|
| E5 合并树编译不过（阻塞） | ✅ 已修 —— npm run build 退出码 0 |
| E1 修复本身 | ✅ 依然成立 |
| E4 测试的辨别力 | ✅ 新代码形状下 7/7 变异体被杀 |
| E6 遗留的产物形状 skill 变得不可管理 | |
| E2 崩溃窗口留两个孤儿、从不清扫 | ❌ 未变 |
E3 合法的 db.backup-2024-06 被静默丢弃 |
❌ 未变 |
E7 — e0f831ed27 有效，但只在 ACP 这条路上
把保留后缀校验从 validateSkillSlug / validateWorkspaceSkillName 移到 install-only 的 helper 里，对 qwen/skills/* 确实达到了预期：滞留的 $QWEN_HOME/skills/foo.backup-1-2/ 又能 toggle 和删除了，而 install 仍然被拒。
注意第三行：project scope 的 setEnabled 仍然报 Project skill not found——这条路是从被过滤的列表里查，而不是直接读目录。
E8 — ⚠️ 走 qwen serve（Web Shell 用的那个面）时，两种 scope 都仍然够不着
DELETE /workspace/skills/:name 不再被名字校验拦下，只是改成在下一步失败：
main |
PR 旧 head | PR d85affc816 |
|
|---|---|---|---|
| 是否被列出 | 是 | 否 | 否 |
DELETE …?scope=workspace |
deleted: true |
invalid_skill_name |
skill_not_found |
DELETE …?scope=global |
deleted: true |
— | skill_not_found |
| 磁盘上是否残留 | — | 是 | 是 |
原因在 WorkspaceService.deleteWorkspaceSkill()（packages/cli/src/serve/workspace-service/index.ts:1048）：它从 getWorkspaceSkillsStatus() 里查 installedPath，查不到就抛 WorkspaceSkillNotFoundError。而新的过滤器恰好把这类名字从那个列表里清空了，所以可达的路由永远解析不到。相对 main 这是 HTTP 面上的行为回退——而且正是这次提交想解开的那个场景。
新加的测试抓不到它，因为它是用显式路径调模块级函数：
// packages/cli/src/serve/workspace-skill-management.test.ts
await expect(
deleteWorkspaceSkill(workspace, 'workspace', name, skillFile), // installedPath 直接传进去
).resolves.toEqual({ skillName: name, scope: 'workspace', deleted: true });真实路由手里没有 skillFile，它要让 service 去找。走 WorkspaceService.deleteWorkspaceSkill(ctx, name, scope) 或者对路由做集成测试，就会红。
另外：这类 skill 在任何地方都不被列出，用户根本无从得知名字，也就没法去调那条仍然可用的 ACP 方法。
E2 / E3 — 未变
在新 head 上重跑，结果一致：在交换点硬杀仍然同时留下 pptx.backup-* 和 pptx.installing-*，skill 仍然从产品里消失而完整副本就在旁边，之后一次成功的重装仍然两个都不清扫（1.2MB → 3.6MB），db.backup-2024-06 仍然被静默丢弃。
E4 — 测试与套件
新代码形状下 7/7 变异体被杀（两处校验现在是独立的 rejectInstallArtifact* helper，所以 M2/M5 回退的是调用点）。packages/core 22328 通过 / 0 失败；packages/cli 26048 通过 / 13 失败——其中 11 条 AuthDialog 超时单独串行重跑 25/25 全过，另外 2 条 SystemController > get_usage_info 在干净的 origin/main 上同样失败。
这个 PR 现在的位置
阻塞项已消除，核心修复扎实，所以想合的话现在可以合。剩下的问题是：每一版补丁堵住一个入口，下一个入口又露出来——E6 → E7 修好了 ACP，E8 又冒出来；E2/E3 则一直在。两条路：
- 最小改动，保持现有形状——给
WorkspaceService.deleteWorkspaceSkill加一条「列表查不到就按目录直接解析」的兜底（照抄readManagedSkillFile里scope === 'global'那个分支，ACP 之所以能用正是因为它），测试改成走 service 而不是模块函数，再补上 E2 的残留清扫。 - 结构性改动，整体更小——像
installWorkspaceSkill那样把暂存和备份放到path.dirname(skillsBaseDir)（即~/.qwen/），并在暂存前清扫残留。loader 只扫<root>/<provider-dir>/skills（以及显式配置的自定义目录），于是没有任何一方需要认识产物形状：两处 loader 过滤、两处保留后缀校验、install-only / management-path 的拆分、E3 的误伤、E8 的滞留全都不必存在，E2 的泄漏也顺带清掉。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
installSkillFromUrl()deletes the existing skill directory before renaming the staged replacement into place. If the rename fails (e.g.EPERMon Windows due to file/indexer/antivirus locks), both copies are lost — the previous installation is gone and the catch handler removes the staging directory. This PR replaces the delete-then-rename sequence with a rollback-capable directory swap: rename the existing skill to a.backup-*sibling, rename staging into place, then either remove the backup on success or restore it on failure.Why it's needed
Fixes #10187. A failed install should leave the previously installed skill intact, but the current code destroys it before attempting the rename. The new rollback path guarantees the old skill survives any rename failure.
Reviewer Test Plan
How to verify
Run the skill-management test suite. The new regression test creates a real skill directory, triggers a reinstall with a mocked failing rename, and asserts the backup is created and cleaned up.
Evidence (Before & After)
N/A — internal rollback logic, no visible UI change.
Tested on
Environment (optional)
Unit tests only.
Risk & Scope
installSkillFromUrl; no public API changes.EPERMtiming with antivirus scanners not simulated in CI.Linked Issues
Fixes #10187
中文说明
这个 PR 做了什么
installSkillFromUrl()在把暂存目录 rename 到正式位置之前，会先删除已有的 skill 目录。如果 rename 失败（例如 Windows 上因文件索引器或杀毒软件导致EPERM），两份副本都会丢失——旧的安装被删掉，catch 又清理了暂存目录。本 PR 将「先删后 rename」替换为可回滚的目录交换：先把已有 skill 重命名为.backup-*兄弟目录，再把暂存目录 rename 到正式位置，成功后删除备份，失败则恢复备份。为什么需要
修复 #10187。安装失败时应保留已安装的 skill，但当前代码在 rename 之前就把它删了。新的回滚路径保证旧 skill 在任何 rename 失败场景下都能存活。
评审者测试计划
如何验证
运行 skill-management 测试套件。新增的回归测试会创建真实 skill 目录，mock rename 失败后触发重装，断言备份被创建并最终清理。
证据（修改前后）
N/A — 内部回滚逻辑，无可见 UI 变化。
测试环境
环境（可选）
仅单元测试。
风险与范围
installSkillFromUrl内部，无公共 API 变更。EPERM时序未在 CI 中模拟。关联 Issue
Fixes #10187