fix(skills): comprehensive review pass — security, correctness, robustness

Eleven findings from /qreview (claude-opus-4-7), grouped by area:

CORRECTNESS

- C1: appendAdditionalContext silently dropped reminders for any tool
  whose llmContent is a single non-array Part (read-file returning
  inlineData for images / PDFs is the canonical case). Both the
  ConditionalRulesRegistry rule reminder and the path-conditional
  skill activation reminder were lost. Wrap the single-Part case
  into an array so the addition still lands.
- S2: Legacy tool-name aliases (`replace` → `edit`,
  `search_file_content` → `grep_search`, `task` → `agent`) bypassed
  FS_PATH_TOOL_NAMES. The registry resolves the alias at execute time
  but `request.name` keeps the alias, so `replace({ file_path: ... })`
  produced empty candidates and missed activation. Canonicalize via
  `ToolNamesMigration` before the allowlist check.
- S5: `new SkillActivationRegistry(...)` ran picomatch unguarded —
  pathological patterns (oversize / broken extglob) could throw and
  abort all of `refreshCache`. Wrap each picomatch call in try/catch
  inside the constructor; drop the bad pattern, keep the rest of
  the skill, log via debugLogger.
- S7: Extension parser (skill-load.ts) silently dropped
  `disable-model-invocation` and `when_to_use`. Now that we have
  `paths:`, that meant an extension SKILL.md with both `paths:` and
  `disable-model-invocation: true` would still fire path-activation
  reminders for a skill the model can't invoke — directly
  contradicting the bug_004 fix at the project/user level.
- S8: SkillTool discarded the `addChangeListener` cleanup function
  and had no `dispose()`. Subagents share the parent's SkillManager
  via `InProcessBackend.createPerAgentConfig`, so each per-subagent
  SkillTool registered another listener; with the listener pipeline
  now async, every path activation serialized through every stale
  subagent's refresh chain. Mirror AgentTool: store the cleanup,
  expose `dispose()`.

SECURITY / SUPPLY-CHAIN

- S11: `validateSkillName`'s `/^[a-zA-Z0-9_:.-]+$/` rejected every
  non-ASCII name on upgrade, silently dropping CJK / Cyrillic /
  accented Latin skills. The structural-injection guard targets
  `<>"'/\n\r\t` etc; entire Unicode planes are not the threat.
  Widen to `/^[\p{L}\p{N}_:.-]+$/u`. Update docs/users/features/
  skills.md to match.
- S10: `parsePathsField` only validated shape (must-be-array). Now
  also reject leading-slash absolute patterns and `..` parent-escape
  patterns at parse time — these silently never match anything in
  the activation registry, so an author who writes `paths:
  ['/etc/passwd']` or `['../*.ts']` would otherwise see the skill in
  /skills and never understand why it never activates.

ROBUSTNESS

- S3: `coreToolScheduler` emitted "skill X is now available via the
  Skill tool" even when the calling subagent's tool registry did not
  expose SkillTool (subagent's `tools:` allowlist excluded `skill`).
  Gate the reminder on `toolRegistry.getTool(ToolNames.SKILL)`.
- S4: `extensionManager.refreshMemory` used `Promise.all` so a
  rejection from skill or subagent refresh nuked the other leg AND
  the hierarchical-memory refresh below it. Switch to
  `Promise.allSettled`, log each rejection, and `await` the
  hierarchical refresh too (the comment justifies awaiting; the
  code didn't).
- S9 / S12: `docs/users/features/skills.md` claimed `paths:` only
  gates model discovery and slash invocation always works. True for
  the user-side path itself, but if the model then tries to chain
  off the user's invocation (call `Skill { skill: ... }` itself),
  validateToolParams returns "gated by path-based activation" —
  contradicting the doc. Rephrase to call out the model-side
  limitation explicitly.

DEFERRED

- S6: notifyChangeListeners swallows per-listener errors and the
  reminder still fires. Real concern but the fix needs an API
  shape change (listener-failure signal back to the scheduler);
  worth its own design discussion. Logged here for follow-up.

Adds 12 regression tests across the 7 affected files. 632 tests
pass; types and lint clean.
This commit is contained in:
Shaojin Wen 2026-05-01 17:21:37 +08:00
parent 2771aac489
commit 09248c0745
13 changed files with 287 additions and 34 deletions

View file

@ -89,12 +89,12 @@ Show concrete examples of using this Skill.
Qwen Code currently validates that:
- `name` is a non-empty string
- `name` is a non-empty string matching `/^[\p{L}\p{N}_:.-]+$/u` — Unicode letters and digits (CJK / Cyrillic / accented Latin all OK), plus `_`, `:`, `.`, `-`. Whitespace, slashes, brackets and other structurally unsafe characters are rejected at parse time.
- `description` is a non-empty string
Recommended conventions (not strictly enforced yet):
Recommended conventions:
- Use lowercase letters, numbers, and hyphens in `name`
- Prefer lowercase ASCII with hyphens for shareable names (e.g. `tsx-helper`)
- Make `description` specific: include both **what** the Skill does and **when** to use it (key words users will naturally mention)
### Optional: gate a Skill on file paths (`paths:`)
@ -115,7 +115,7 @@ Notes:
- Globs are matched relative to the project root with [picomatch](https://github.com/micromatch/picomatch); files outside the project root never trigger activation.
- A path-gated Skill **stays activated for the rest of the session** once a matching file is touched. A new session, or a `refreshCache` triggered by editing any Skill file, resets activations.
- `paths:` only gates **model** discovery. You can still invoke a path-gated Skill yourself at any time via `/<skill-name>` or the `/skills` picker — gating does not require waiting for activation.
- `paths:` only gates **model** discovery, and only at the SkillTool listing level. You can always invoke a path-gated Skill yourself via `/<skill-name>` or the `/skills` picker — that user path runs the Skill body regardless of activation state. The model side, however, stays gated until a matching file is touched: a slash invocation does **not** unlock model-side activation, so if you want the model to chain off your invocation (call `Skill { skill: ... }` itself), also access a file matching the skill's `paths:` first.
- Combining `paths:` with `disable-model-invocation: true` is allowed but the gate has no effect — the Skill is hidden from the model regardless, so path activation never advertises it.
## Add supporting files