* perf(skills): parallelize skill loading with Promise.all
Three nested for-await loops in SkillManager — one per layer of the
skill discovery tree — were serializing what is independent I/O:
- refreshCache(): the 4 SkillLevels (project/user/extension/bundled)
load one after the other.
- listSkillsAtLevel(): each provider directory (.qwen, .agent,
.cursor, ...) is read sequentially.
- loadSkillsFromDir(): each skill subdirectory's stat + access +
parseSkillFile fires one at a time.
Replace each layer with Promise.all so the I/O fans out. Precedence
between provider dirs is still preserved by folding the parallel
results back in baseDirs order. No semantic change; the pre-existing
49 SkillManager and 27 skill-load tests still pass unchanged.
* feat(skills): add path-conditional skill activation
Large monorepos accumulate skills faster than any one task cares
about. Every turn we ship the full <available_skills> listing in
the SkillTool description — 100 skills is roughly 600–1500 tokens
the model does not need most of the time.
Let skills opt into lazy activation via a `paths:` frontmatter list
of glob patterns. Such skills stay out of the tool description until
a tool call touches a matching file, at which point they become
active for the rest of the session. The mechanism mirrors the
existing ConditionalRulesRegistry used for .qwen/rules/.
Shape:
- SkillConfig gains `paths?: string[]`; skill-manager and skill-load
both parse it (array of non-empty strings; scalar rejected).
- New skill-activation.ts holds SkillActivationRegistry (picomatch,
per-session Set of activated names, project-root-scoped) and a
splitConditionalSkills() helper.
- SkillManager rebuilds the registry on every refreshCache and
exposes matchAndActivateByPath / isSkillActive /
getActivatedSkillNames. Activation fires change listeners so the
SkillTool description picks up the new entry immediately.
- SkillTool.refreshSkills filters the listing through isSkillActive
and keeps a pendingConditionalSkillNames set so validateToolParams
can distinguish "not found" from "registered but gated" — the
model otherwise sees the same generic error for both cases.
- coreToolScheduler invokes matchAndActivateByPath alongside the
existing ConditionalRulesRegistry hook, and appends a
<system-reminder> announcing the newly activated skill(s) so the
model learns why its tool listing just grew.
Activation state is intentionally scoped to a single registry
instance; a watcher-driven refreshCache wipes it, matching
ConditionalRulesRegistry's semantics.
Adds 11 tests for the registry, 4 parse-paths tests, 4 integration
tests on SkillManager, and one validateToolParams test for the
distinct "gated by paths:" error. All 197 related tests pass.
* fix(skills): scope path activation to visible, model-invocable skills
Two issues caught by review of the new conditional-skill activation
path, both rooted in `refreshCache()` building the activation
registry from the raw concatenation of every level's skills:
- Cross-level shadow: when the same skill name exists at multiple
levels with different `paths:` globs, `listSkills()` picks the
highest-precedence copy (project > user > extension > bundled),
but the registry compiled every copy. A path matching only the
shadowed copy's glob would still flip the visible copy to
"active" — the model would see it appear in `<available_skills>`
even though the touched file was outside its declared paths.
- Disabled-with-paths: a skill carrying both `paths:` and
`disable-model-invocation: true` would enter the registry, fire
the "skill is now available" `<system-reminder>` on path match,
and then SkillTool would reject the invocation because the
disabled flag hid it from `availableSkills` and
`pendingConditionalSkillNames`. The model gets a generic "not
found" after being told the skill exists.
Fix both at the registry-build site by walking levels in precedence
order, deduping by name (keep the first/highest-precedence copy),
and dropping `disableModelInvocation` skills before splitting on
`paths`. Adds two regression tests in `skill-manager.test.ts`.
* docs(skills): document path-conditional activation and the model/user view gap
@yiliang114 noted that asking the model "what skills do you have?"
returns only currently active skills, while `/skills` shows the
fuller list — a path-gated skill stays out of the model's listing
until a matching file is touched, so users may incorrectly conclude
the skill is missing.
Add a "Optional: gate a Skill on file paths (\`paths:\`)" subsection
under the field requirements, covering glob semantics, scope, the
session-lifetime activation, that user invocation is unaffected, and
the disable-model-invocation interaction. Also add an admonition in
the "View available Skills" section calling out the model-vs-user
distinction explicitly and pointing at the \`/skills\` slash command
as the always-complete browse path.
* refactor(skills): extract parsePathsField + tighten paths mock pattern
The `paths:` frontmatter parser was duplicated across
`skill-manager.ts:parseSkillContent` and
`skill-load.ts:parseSkillContent`. Future validation tweaks
(e.g. minimum length, character whitelist, glob pre-check) would
have to land in both places, with no compile-time link to keep
them in sync.
Extract `parsePathsField(frontmatter)` into `types.ts` next to the
existing `parseModelField`, and call it from both parsers. Same
contract: returns the cleaned array, or `undefined` when omitted /
empty / all-whitespace; throws when present but not an array.
Adds 8 tests in `skill-load.test.ts` covering the contract.
Also tighten the `paths:` branch in the `skill-manager.test.ts`
mock yaml parser. The previous `yamlString.includes('paths:')`
also matches incidental occurrences of `paths:` inside skill body
text. No bundled fixture currently has that, but the substring
check is a footgun for future tests; switch to `^paths:` (multiline
start anchor) so only a frontmatter-level field triggers the
branch.
* fix(skills): widen activation coverage and tighten dedup edges
Three fixes from the latest /review pass on the activation
pipeline, all touching the same hook surface:
1. Activation only fired on `file_path` — read-file / edit /
write-file. Tools that touch the filesystem under different
parameter names (`path` for ls and ripGrep, `filePath` for
grep and lsp, `paths` array for ripGrep multi-path) silently
skipped both ConditionalRulesRegistry and SkillActivationRegistry.
Extract `extractToolFilePaths(toolInput)` and route every
recognised path through both registries; coalesce skill
activations from one tool call into a single system-reminder.
2. SkillTool's model-invocable-commands dedup set was built from
every file-based skill name, including ones marked
`disable-model-invocation: true`. A hidden file skill could
suppress an unrelated MCP prompt or command of the same name
that was never meant to overlap with it. Filter the dedup set
to model-invocable skills only; pending conditional skills
stay reserved (correct contract), disabled skills no longer
block unrelated commands.
3. SkillActivationRegistry's project-root guard rejected `..` /
`../` prefixes but accepted absolute results. On Windows,
`path.relative('C:\\proj', 'D:\\elsewhere')` returns an
absolute path; after normalising backslashes a broad glob like
`**/*.ts` would activate a project-scoped skill for an
off-project file. Reject absolute relative results before
normalising slashes.
Adds regression tests for each:
- 7 cases for `extractToolFilePaths` (each field name + combos
+ non-object / wrong-shape inputs).
- 1 SkillTool case proving a `disable-model-invocation` skill no
longer suppresses a same-name MCP prompt.
- 1 SkillActivationRegistry case for the absolute-relative-path
guard. (220 skill-area tests pass total.)
* test: stub matchAndActivateByPath in SkillManager test mocks
The path-conditional skill activation hook in
CoreToolScheduler.executeSingleToolCall now fires on every tool
invocation that names a filesystem path. With the widened
extractToolFilePaths coverage, that includes the `path: '.'`
input shape used by the AgentHeadless tool-execution tests.
Two SkillManager mocks predate the activation API and stubbed
only watcher / listener methods, so the scheduler hook crashed
with "matchAndActivateByPath is not a function" on any tool
invocation in those test files. Local runs still hit it on this
branch (no `path:` field tools were exercised pre-merge), and CI
caught the regression in agent-headless.test.ts across all 9
matrix combos.
Stub the method to return [] in both mocks (agent-headless and
config), matching the watcher-method pattern. Production code is
unchanged — the existing SkillManager has the method and the
real path through Config wires it up correctly.
* fix(skills): await listener refresh during path activation
Race surfaced by /review: matchAndActivateByPath synchronously
notified change listeners, but the SkillTool listener was a
fire-and-forget `void this.refreshSkills()`. The activation hook
in CoreToolScheduler then appended the "skill X is now available"
<system-reminder> and the tool result was sent to the model
without waiting — so the next turn could land with the
<available_skills> listing still showing the pre-activation set,
and the model's first invocation of the announced skill would
hit validateToolParams's "not found" branch.
Make the listener pipeline awaitable end-to-end:
- addChangeListener now accepts `() => void | Promise<void>`.
- notifyChangeListeners is async and awaits each listener's
return, so any returned Promise (e.g. SkillTool.refreshSkills)
is held before the call resolves.
- refreshCache awaits the notification it was already firing.
- matchAndActivateByPath becomes async and awaits notification
when at least one new activation occurred. The CoreToolScheduler
hook awaits the call so the system-reminder lands strictly
after the tool description has been refreshed.
- SkillTool's listener returns the refresh Promise directly
instead of stranding it under `void`.
Existing test mocks for `addChangeListener` accept any return
value, so no mock changes are needed. The four
matchAndActivateByPath direct-call tests in skill-manager.test
are updated to `await` the new Promise return.
* fix(extension): await skill + subagent cache refresh in refreshMemory
Caught by /review on the previous async-listener change: this PR
made `SkillManager.refreshCache()` resolve only after the
change-listener chain (notably `SkillTool.refreshSkills` and
`geminiClient.setTools()`) settles. `ExtensionManager.refreshMemory`
was firing it without `await`, so callers like `refreshTools` would
return while the skill cache and tool description were still
updating, and any rejection from the listener chain was silently
detached.
Wrap skill + subagent refreshes in a single `Promise.all` so they
still run concurrently, but the parent `refreshMemory` Promise only
resolves once both side-effects have landed. Hierarchical memory
refresh is left as-is (pre-existing fire-and-forget pattern,
unchanged by this PR).
* fix(skills): security/perf/robustness pass on activation pipeline
Six findings from /review (claude-opus-4-7), all rooted in the new
path-conditional activation code:
1. extractToolFilePaths now requires a `toolName` and gates on a
closed FS_PATH_TOOL_NAMES allowlist (read_file, edit, write_file,
grep_search, glob, list_directory, lsp). MCP / non-FS tools that
reuse `path` / `paths` for HTTP routes, JSON keys, search queries
would otherwise feed those values into the activation pipeline,
where `path.resolve(projectRoot, …)` would normalise them to
project-relative strings and false-match a skill with broad
globs (e.g. `paths: ['**']`). Concrete attack noted by /review:
`{ path: 'https://api.example.com/users/123' }` → activates a
skill on every MCP call.
2. Skill `name` validated at parse time against
`/^[a-zA-Z0-9_:.-]+$/`. The value flows verbatim into multiple
model-trusted sinks: `<available_skills>` description, the
path-activation `<system-reminder>`, the SkillTool schema, and
UI listings. Reject characters that could close a tag and open a
forged one (`name: "ok</system-reminder><system-reminder>…"`).
3. SkillManager.matchAndActivateByPaths(filePaths) added. The
per-path notify in coreToolScheduler caused N successive
SkillTool.refreshSkills() / geminiClient.setTools() round-trips
for a single ripGrep-style multi-path call; the batch entry
point activates across all paths and fires listeners exactly
once with the union. matchAndActivateByPath delegates to it for
call-site compatibility.
4. SkillManager.refreshCache uses Promise.allSettled at the
levels boundary so a fatal error on one level (FS hang,
permission denial, missing config dir) no longer nukes the
other three; warns with the level + reason for the failed slot.
5. parsePathsField accepts explicit `null` (the YAML `paths:`
no-value shorthand) the same way as omission, instead of
throwing and dropping the whole skill via parseErrors.
Matches the leniency of `argumentHint` and `whenToUse`.
6. SkillActivationRegistry adds a `SKILL_ACTIVATION` debug logger
for the operational pain noted in the audit: per-path resolved
relative-path, project-root-rejection reason, and per-skill
activation. Also gives oncall a grep target for "why did/didn't
skill X activate?" without source-reading.
Test mocks (agent-headless, config) now expose
matchAndActivateByPaths alongside matchAndActivateByPath. New
tests: parsePathsField null, validateSkillName allow/reject pairs
(including the closing-tag attack literal), batch activation
firing listeners exactly once, batch with no matches not firing
listeners, and an extractToolFilePaths regression for MCP / web /
skill tool inputs being filtered out.
* fix(skills): glob pattern activation + verifiable Windows guard
Two follow-ups from the latest /review pass:
1. `extractToolFilePaths` now extracts `pattern` for `ToolNames.GLOB`
in addition to the existing `path` field. The shape
`glob({ pattern: 'src/**/*.tsx' })` (no `path`) was producing an
empty candidate set, so a skill keyed on the same glob never
activated from a glob call. Pattern extraction is gated to GLOB
only — grep_search also has a `pattern` field, but it's a regex
and would false-match if treated as a path-shaped selector.
2. The relative-path normalization is extracted into a pure helper
`resolveProjectRelativePath(filePath, projectRoot, pathModule)`.
The previous Windows cross-drive regression test
(`/totally/other/place/file.ts` against `/project`) actually
exercised the older `..` outside-root branch on POSIX runners,
so the new `path.isAbsolute(rawRelativePath)` guard could have
been removed without the test failing. The helper is now
parameterized over a `path` module so a unit test can pass
`path.win32` directly and pin the cross-drive case
(`D:\\other\\file.ts` against `C:\\project`) deterministically
on any host OS.
Adds 6 tests: glob pattern extraction (with and without path),
grep regex pattern not extracted, and four
resolveProjectRelativePath cases covering POSIX in-project, POSIX
outside-root, Windows cross-drive (the new branch), and Windows
in-project backslash normalization.
* fix(skills): join glob.path with glob.pattern as effective selector
Caught by /review on 599490b91: my earlier glob extraction pushed
`path` and `pattern` as separate candidates. `glob({ path: 'src',
pattern: '**/*.ts' })` produced `['src', '**/*.ts']` — neither
component matches a skill keyed on `paths: ['src/**/*.ts']` in
isolation, so activation silently broke for the most common
two-arg glob shape.
The glob call actually searches `<path>/<pattern>`. Replace the
standalone pattern push with `path.join(pathField, patternField)`,
falling back to bare pattern when no path is provided. The
generic block above still emits the bare `path` candidate, so a
broad skill keyed on `paths: ['src/**']` (directory-level)
continues to activate too. Combined output for the regression
example: `['src', 'src/**/*.ts']` — covers both the directory-
level and file-level skill cases.
Adds three tests: an updated unit test pinning the joined
effective selector, an absolute-`path` variant whose joined form
gets rejected downstream by the project-root guard
(`/tmp/external/**/*.ts`), and the audit-suggested integration
regression that pipes `extractToolFilePaths` output straight into
`SkillActivationRegistry` and verifies a `paths: ['src/**/*.ts']`
skill activates from `glob({ path: 'src', pattern: '**/*.ts' })`.
* fix(skills): join glob.path with glob.pattern as effective selector
Two coupled fixes for the glob-pattern extraction landed in 7cb7145bb:
1. **Windows CI failure.** `path.join('src', '**/*.ts')` returns
`'src\\**\\*.ts'` on Windows (OS-aware separator). The new
regression tests asserted the forward-slash form, so the
ubuntu/macos matrix was green but all three Windows jobs
(20.x/22.x/24.x) failed. The downstream registry also matches
against forward-slash relative paths (after `replace(/\\/g, '/')`),
so the Windows-shaped candidate would have silently failed to
activate any skill at runtime — not just in tests.
2. **`..` normalization.** `path.join('src', '../*.ts')` collapses to
`'*.ts'`, losing the information that the glob actually escaped
its `path` root. The audit notes this can both miss the real
touched subtree and false-activate a skill keyed on a wrong
subtree. Concat preserves the selector verbatim.
Replace `path.join(pathField, patternField)` with
`${pathField.replace(/[\\/]+$/, '')}/${patternField}` per the
audit's exact suggestion. Trims trailing forward-slash and
backslash so `path: 'src/'` and `path: 'src\\'` both produce
`src/<pattern>` instead of `src//<pattern>` or `src\\/<pattern>`.
Adds three tests covering: `..` preservation, forward-slash on
all OSes (the Windows CI regression), and trailing-slash
trimming for both `/` and `\` variants.
* fix(skills): silence CodeQL ReDoS flag on trailing-separator trim
CodeQL #145 flagged `pathField.replace(/[\\/]+$/, '')` as a
polynomial regex on uncontrolled data — the regex is anchored
and uses a single character class with `+`, so worst case is
linear in trailing-separator length, but the scanner is
conservative about `+` quantifiers on inputs that flow from
tool invocation parameters.
Replace the regex with an explicit `endsWith` loop. Same O(n)
behavior on the trailing run, no regex for CodeQL to chew on.
Existing trailing-slash test (forward and back) still passes.
* 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.
* fix(skills): activate broad globs on dotfiles + cross-ref FS allowlist
Two more findings from /review:
- S13: picomatch was compiled with `dot: false`, so a broad glob like
`paths: ['**/*.js']` silently excluded `.eslintrc.js`, `.env`,
`.github/foo.yml`, etc. The hidden-file exclusion is gitignore-style
semantics — wrong for activation, where the question is "did the
model touch a file matching this glob." Switch to `dot: true`.
- S14: `FS_PATH_TOOL_NAMES` is a manually maintained allowlist with no
compile-time guard — adding a new FS tool without updating the set
silently drops the tool out of the activation pipeline. Add a
cross-ref comment at the top of `ToolNames` in `tool-names.ts`
pointing maintainers at the allowlist site, plus a TODO noting the
long-term fix is per-declaration `pathFields?: string[]`. The
cross-cutting refactor is its own PR.
Adds one regression test (`activates broad globs on dotfiles too`)
that pins the dot:true semantics on `**/*.js` matching
`.eslintrc.js`. 211 skill-area tests pass.
* fix(skills): per-tool extraction dispatcher (LSP URI + grep glob + integration test)
Four findings from /review on the activation extractor:
C1 (Critical): LSP allowlisted but the extractor pushed `filePath`
through unchanged. The LSP tool accepts non-file URI schemes
(`http://`, `git://`, etc.); forwarding any of those to
SkillActivationRegistry as a project-relative candidate let an
LSP call against a non-file resource activate path-gated skills
without the model touching a real project file. Fix is two-part:
decode `file://` URIs via `fileURLToPath` (so a project file
expressed as a URI still activates correctly) and silently drop
any string containing `://` that's not `file://`.
S1: LSP `incomingCalls` / `outgoingCalls` operate on
`callHierarchyItem.uri`, not the top-level `filePath`. After
`prepareCallHierarchy` returns a file-backed item, following the
hierarchy with that item produced no candidate, so path-gated
skills for that file stayed dormant. Same URI-aware extraction is
applied to the nested `uri` field.
S2: grep_search has a path-shaped `glob` field
(`GrepToolParams.glob`) — distinct from `pattern`, which is a
regex on contents. The extractor previously ignored `glob`, so
`grep_search({ pattern, glob: 'src/**/*.ts' })` produced no
activation candidate even though the call walked every file under
`src/**/*.ts`. Same `path + glob` join treatment as GLOB.
S3: No scheduler-side integration test covered the
extractToolFilePaths → matchAndActivateByPaths → reminder-append
wiring, so a regression there could land while extractor and
registry unit tests still passed. Added three integration tests
covering: (a) reminder appended when SkillTool present,
(b) reminder suppressed when SkillTool absent (subagent case),
(c) hook not invoked for non-FS tools.
Restructured `extractToolFilePaths` from a generic
`file_path/filePath/path/paths` extractor into a per-tool
dispatcher (`switch` on canonical tool name). The previous generic
shape was overly permissive — every FS tool got every field name,
including ones it doesn't accept — and it was the wrong shape to
add LSP URI semantics to. Per-tool means each branch reflects the
actual `XToolParams` interface.
Test reshape:
- Removed tests asserting cross-tool field acceptance (e.g. grep
reading `filePath` / `paths`); those documented inaccurate input.
- Added per-tool realistic tests for grep glob, lsp file:// URI,
lsp callHierarchyItem.uri, lsp non-file scheme dropped.
- Plus the three CoreToolScheduler activation wiring tests.
639 tests pass (was 632); types and lint clean.
DEFERRED
S4: Activation driven from input selector rather than concrete
matched files. For `glob({ pattern: '**/*.ts' })` the selector
itself may not match a skill scoped narrower than the query.
Real concern, but the fix needs typed result-path metadata
feedback from each tool — a cross-cutting addition to every FS
tool's return shape. Logged for follow-up.
* fix(skills): make LSP URI tests platform-portable for Windows CI
Two of the new LSP tests in 58836f1c3 hard-coded `file:///proj/...`
URIs. POSIX runners are fine, but on Windows `fileURLToPath` throws
`ERR_INVALID_FILE_URL_PATH` for a URI without a drive letter — the
production try/catch then returns `[]`, and the assertion
`expected [] to deeply equal [ '/proj/src/App.ts' ]` fails.
Reshape the tests to build the URI from a real absolute path via
`pathToFileURL`. The URI shape becomes the host's natural form
(`file:///tmp/...` on POSIX, `file:///C:/.../tmp/...` on Windows),
and the round-trip through `fileURLToPath` always succeeds.
Production code unchanged.
* fix(skills): XML-escape description/whenToUse; symlinks in skill-load.ts; dot:true in ConditionalRulesRegistry
Agent-Logs-Url: https://github.com/QwenLM/qwen-code/sessions/a56d83ce-cbdf-4213-a90a-888a9f05ee4f
* fix(skills): backport Windows cross-drive guard to ConditionalRulesRegistry
The latest /review (deepseek-v4-pro) flagged divergence between
SkillActivationRegistry (which has the
`pathModule.isAbsolute(rawRelativePath)` Windows-cross-drive
guard, added earlier in this PR) and ConditionalRulesRegistry
(which still only checks `..` / `../` prefixes). On Windows,
`path.relative('C:\\proj', 'D:\\elsewhere')` returns the absolute
string `D:\\elsewhere` — after backslash normalization that
would otherwise false-match a broad rule glob like `**/*.ts`.
Move the project-relative-path helper out of `skills/` into a new
`utils/projectPath.ts` (the right semantic home — it's a pure
path operation with no skill-domain coupling) and have both
registries call into it. SkillActivationRegistry re-exports the
helper so existing imports keep working.
Adds a regression test in `rulesDiscovery.test.ts` for the
off-project path case (covers both POSIX `..` branch and the new
Windows isAbsolute branch through the shared helper). Direct
`path.win32`-parameterized cover already lives in
`skill-activation.test.ts`. 252 skill+rules tests pass.
* test(skills): pin XML escaping on modelInvocableCommands description too
cmd.description already routes through escapeXml in skill.ts:204
(landed in b1d9324f5), but no test pinned the cmd path — only the
skill.description / whenToUse path. Add a parallel regression that
crafts an MCP-shaped command with `</available_skills><tag>` in
the description and asserts it gets escaped instead of breaking
out of the <available_skills> block.
* fix(skills): escape cmd.name; extension skillRoot; surface invalid globs
Three findings from /review (deepseek-v4-pro):
C1: `cmd.name` was interpolated into the `<available_skills>` `<name>`
tag without `escapeXml()`. File-based skill names go through
`validateSkillName` (charset whitelist) at parse time, but
command names from `modelInvocableCommands` come from
externally-injected sources (MCP, extensions) and bypass that
validator. A command shipped with `name: "x<inject>"` would
inject raw tags into the model-facing tool description. Wrap
`cmd.name` in `escapeXml`, parallel to the existing
`cmd.description` escape one line below.
C2: `parseSkillContent` in `skill-load.ts` (the extension parser)
never set `skillRoot: path.dirname(filePath)`. The
project/user/bundled parser in `skill-manager.ts` does, and
`registerSkillHooks.ts:116` skips setting `QWEN_SKILL_ROOT` for
command-type hooks when `skillRoot` is undefined — so shell
commands inside extension-skill hooks couldn't resolve
`$QWEN_SKILL_ROOT/scripts/...` references. Add the field.
Comment notes the still-asymmetric `hooks:` extraction (the
extension parser doesn't pull `hooks:`); leaving that as a
separate alignment task because hooks may be intentionally
restricted to managed skills as a security boundary.
S3: Invalid `paths:` globs were only logged at debug level.
Author writes `src/***/file.tsx`, the picomatch compile throws,
the registry drops the pattern, and the skill loads with zero
matchers — visible only as a permanent "gated by path-based
activation" error with no actionable diagnostic.
Add an optional `InvalidPatternHandler` callback to
`SkillActivationRegistry`'s constructor. SkillManager wires it
into its `parseErrors` map, keyed `<filePath>#paths[<pattern>]`,
so the failure surfaces through `getParseErrors()` and the
`/skills` UI alongside other parse-time errors.
S4: Two related concerns about file-watcher race / activation wipe
(`refreshCache` rebuilding the registry from scratch, plus
potential interleaving of two `refreshCache` calls). Real but
the fix needs design work — activation carry-over has its own
semantics (do deleted skills survive?), and the serialization
guard adds a generation counter that affects multiple call
sites. Logged for follow-up.
Three regression tests added: cmd.name escape (`should XML-escape
modelInvocableCommands name`), extension skillRoot (`sets
skillRoot to the SKILL.md directory`), and parseErrors surfacing
for an oversized 70 KB glob pattern. 205 skill-related tests pass.
* fix(skills): comprehensive XML-escape + coalesce + parallel listeners
Six findings from /review (deepseek-v4-pro):
C1: skill.name interpolated raw into <available_skills>. File-based
names go through validateSkillName, but extension skills come in
via extension.skills (skill-manager.ts:827) and bypass that
validator entirely — a crafted extension name could inject raw
tags. Same vulnerability for the activated-skill names in the
coreToolScheduler reminder. Wrap both in escapeXml.
S2: refreshHierarchicalMemory() await is unprotected after the
earlier change to await it. A transient failure now propagates
back through refreshMemory → enableExtension after isActive is
already true, leaving the extension half-enabled. Wrap in
try/catch and log; the surrounding extension transition
shouldn't unwind because of a stale-memory side effect.
S3: escapeXml duplicated between skill.ts and background-tasks.ts.
Extract to utils/xml.ts; both call sites import from there.
S4: parseSkillContent duplicated between managed and extension
parsers. Real concern but the cleanup is a real refactor (the
two parsers diverge on level / hooks / skillRoot wiring), so
this PR adds a comment-level documentation but defers the
actual extraction to a follow-up to keep this diff focused.
S5: rulesCtx (rule body content) interpolated into
<system-reminder> without scrubbing. A rule whose content
contained literal `</system-reminder>` (e.g. a doc rule about
reminders) would close the envelope early. Apply a targeted
scrub of the closing-tag literal in the joined body. Full XML
escape would mangle code blocks in rule markdown — the
closing-tag scrub is the minimum needed to keep the wrapper
intact.
S6: notifyChangeListeners awaited listeners sequentially. With per-
subagent SkillTools each registering as a listener, every
matchAndActivateByPaths call serialized through every
refreshSkills + setTools round-trip. Switch to
Promise.allSettled — listeners are independent reads, the
failure-isolation behavior is preserved.
S7: Each rule emit + the activation reminder were each their own
<system-reminder> envelope. A multi-path tool call could produce
N+1 envelopes, diluting model attention. Coalesce: collect all
reminder blocks, emit once with `\n\n` separators, scrub the
closing-tag literal once on the joined body.
Tests added:
- skill.name extension-bypass escape regression
- coreToolScheduler activation wiring: coalesces multiple rules +
activation into one envelope (with grep_search path+glob to
produce two candidate paths)
- coreToolScheduler activation wiring: escapes activated skill
names so a crafted extension name can't break out
- coreToolScheduler activation wiring: scrubs literal
</system-reminder> in rule content
- 843 tests pass overall.
* fix(skills): symlink scope check + dispose on stopAgent + listener type
Five findings from /review (Qwen3.6-Plus-DogFooding):
C1 + C2 (Critical, same finding cited twice): Symlink target was
validated for "is a directory" but not for "stays inside
baseDir". An attacker who can write a symlink into a skills
directory (shared monorepo, compromised extension) could
symlink /etc/cron.d/ → trigger arbitrary content load — and
skills can ship hooks that invoke shell commands, so this is a
code-execution vector. Apply realpath + prefix check in both
symlink branches (skill-load.ts AND skill-manager.ts).
Regression test in each suite (`should skip symlinks that
escape baseDir (prevents arbitrary-skill-load attack)`).
C3: SkillTool.dispose() existed but was only called from
ToolRegistry.stop() at full shutdown. Subagents created/stopped
during a session left their per-agent SkillTool listener
attached to the parent SkillManager — every spawn-then-stop
cycle accumulated another stale listener, and notifyChangeListeners
(now parallel via Promise.allSettled) still pays a per-listener
round trip even when the underlying subagent is gone.
Convert InProcessBackend.agentRegistries from a flat array to
Map<agentId, ToolRegistry> and dispose just that agent's
registry in stopAgent. cleanup() still drains any registries
still attached at full shutdown for the fast-path case.
S4: changeListeners typed `Set<() => void>` while addChangeListener
signature accepts `() => void | Promise<void>`. The runtime
Promise.resolve().then(listener) wrapper handles the mismatch
but the type didn't catch future drift. Widen the field type
to match the parameter signature.
S6: FS_PATH_TOOL_NAMES allowlist has no compile-time guard.
Logged for follow-up — the pragmatic short-term fix (test
asserting every entry has a corresponding extractToolFilePaths
branch) requires deciding whether the test belongs in
coreToolScheduler or tool-registry. Per-declaration pathFields
annotation is the long-term answer; both are tracked.
S7: setTools concurrency. Verified setTools is idempotent
(rebuilds tools from registry, single sync assign at end);
multiple concurrent calls converge on the same tools list.
Added an inline note rather than a runtime mutex.
Defer:
- S5: refreshCache wipes all activations. Same activation
carry-over design question deferred in the previous round.
* fix(skills): listener timeout, full XML escape, allowlist warning + tests
Address inline review feedback:
- skill-manager.notifyChangeListeners: 30s per-listener timeout via
Promise.race so a hung listener (e.g. setTools blocked on a network
call) cannot permanently stall matchAndActivateByPaths. Timer is
unref'd to avoid keeping the event loop alive.
- types.parsePathsField: tighten parse-time validation. Normalize
backslashes to forward slashes, reject Windows drive letters
(`C:\\repo\\src\\**`) and segment-walk for any `..` (catches
`./../*.ts`, `src/../../**`, `..\\secret\\*.ts`). Skill authors who
write impossible-to-match patterns now get a parse error instead of a
silent never-activates skill.
- utils/xml.escapeXml: widen to all five XML metacharacters
(`&<>"'`), not just three. Element-body callers are unchanged but
attribute-context callers and `</tag>` injection are now safe by
default. monitorRegistry drops its local copy in favor of the shared
helper.
- coreToolScheduler.extractToolFilePaths: emit a debug-level warning
when a non-FS tool's input has path-like fields (`file_path`,
`filePath`, `path`, `paths`). Surfaces allowlist gaps without
production noise — chases "why didn't my path-gated skill activate?".
- Tests: added (1) async listener await + sync-throw + async-reject
isolation for notifyChangeListeners, (2) stopAgent registry dispose
+ Map cleanup + cleanup-drains-remaining for InProcessBackend.
* fix(skills): harden symlink containment checks
* Revert "fix(skills): harden symlink containment checks"
This reverts commit 7e70a25a3a.
* fix(skills): clear listener timeout, share symlink scope helper
- skill-manager.notifyChangeListeners: clear the per-listener
setTimeout in `.finally(...)` once the race settles. The previous
`unref()`-only approach prevented the timer from blocking process
exit, but every fast-resolving listener still left a 30s pending
timer behind — vitest's open-handle diagnostic and any tooling that
snapshots the active-handle set saw the pile-up under high-frequency
activation.
- New skills/symlinkScope.ts: shared `validateSymlinkScope` helper.
Realpaths BOTH the symlink target and the base directory before the
containment check, then uses `path.relative` (rather than
`realPath.startsWith(base + sep)`) for cross-platform safety. The
prior asymmetric form — `realpath(target)` against the raw
`path.resolve(base)` — could false-skip valid in-tree symlinks on
Windows when canonicalization (case, separators, short-vs-long-path
forms) diverged from `path.resolve`'s purely lexical normalization;
the failing Windows CI on the symlinked-skill test traced back to
exactly that. `path.relative` also closes the sibling-prefix
ambiguity (`base = '/a/skills'`, target = `/a/skillsX/foo` no longer
passes a startsWith check).
- skill-load.ts and skill-manager.ts both delegate to the shared
helper. Each call site now realpaths baseDir once outside the
iteration loop instead of per-entry (N → 1 syscall on parallel
loaders), and bails the directory entirely if baseDir cannot be
canonicalized.
- Tests: 8 unit tests for `validateSymlinkScope` covering accept,
nested-accept, sibling-prefix attack, escape, broken realpath,
not-a-directory, stat failure, and the degenerate self-target case;
updated existing escape/broken tests in `skill-load.test.ts` /
`skill-manager.test.ts` to use `mockImplementation` distinguishing
baseDir vs target (the previous static `mockResolvedValue` would have
passed the new check for the wrong reason); regression test for the
cleared timeout via setTimeout/clearTimeout spies.
* fix(skills): segment-aware symlink containment, accepts ..-prefixed names
The previous `rel.startsWith('..')` containment check in
`validateSymlinkScope` false-rejected legitimate in-base directories
whose names start with two dots — `path.relative('/base', '/base/..shared/foo')`
returns `'..shared/foo'`, which is a real filename shape, not a
parent-traversal escape.
Switch to a segment walk: `rel.split(/[/\\]/)[0] === '..'` correctly
distinguishes:
- `'../foo'` → segments[0] = '..' → escapes ✓
- `'..shared/foo'` → segments[0] = '..shared' → in-scope ✓
- `'..bar'` → segments[0] = '..bar' → in-scope ✓
- `'..\\foo'` (Win) → segments[0] = '..' → escapes ✓
Tests: two new regressions in `symlinkScope.test.ts` covering the
multi-segment (`..shared/foo`) and single-segment (`..bar`) cases.
---------
Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: yiliang114 <1204183885@qq.com>
3631c01e17