diff --git a/docs/users/features/skills.md b/docs/users/features/skills.md index 2f2f64ccd..eef367291 100644 --- a/docs/users/features/skills.md +++ b/docs/users/features/skills.md @@ -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 `/` 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 `/` 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 diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index f418c3b23..41a4104cf 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -4464,6 +4464,21 @@ describe('extractToolFilePaths', () => { ).toEqual(['/proj/a.ts']); }); + it('canonicalizes legacy tool-name aliases before the allowlist check', () => { + // Regression: the tool registry resolves `replace` → `edit`, + // `search_file_content` → `grep_search`, etc. at execution time, so + // a model call like `replace({ file_path: 'src/App.tsx' })` actually + // runs EditTool. If the activation pipeline gates on the raw alias + // name, conditional rules and skill activation silently skip every + // tool call that uses a legacy name. + expect( + extractToolFilePaths('replace', { file_path: '/proj/a.ts' }), + ).toEqual(['/proj/a.ts']); + expect( + extractToolFilePaths('search_file_content', { filePath: '/proj/b.ts' }), + ).toEqual(['/proj/b.ts']); + }); + it('returns empty for tool names outside the FS allowlist', () => { // Regression: MCP tools and other non-FS tools that happen to use // `path` / `paths` for non-filesystem semantics (e.g. URL routes, diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index a74b8442d..5deb38f27 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -47,7 +47,7 @@ import type { Part, PartListUnion, } from '@google/genai'; -import { ToolNames } from '../tools/tool-names.js'; +import { ToolNames, ToolNamesMigration } from '../tools/tool-names.js'; import { CONCURRENCY_SAFE_KINDS } from '../tools/tools.js'; import { isShellCommandReadOnly } from '../utils/shellReadOnlyChecker.js'; import { stripShellWrapper } from '../utils/shell-utils.js'; @@ -232,7 +232,14 @@ export function extractToolFilePaths( toolName: string, toolInput: unknown, ): string[] { - if (!FS_PATH_TOOL_NAMES.has(toolName)) return []; + // Canonicalize legacy aliases (e.g. `replace` → `edit`, + // `search_file_content` → `grep_search`) before the allowlist check. + // The tool registry resolves these at execution time, so a tool call + // like `replace({ file_path: 'src/App.tsx' })` actually runs EditTool; + // gating only on the canonical name closes the alias-bypass hole. + const canonical = + (ToolNamesMigration as Record)[toolName] ?? toolName; + if (!FS_PATH_TOOL_NAMES.has(canonical)) return []; if (!toolInput || typeof toolInput !== 'object') return []; const out: string[] = []; const push = (v: unknown): void => { @@ -246,7 +253,7 @@ export function extractToolFilePaths( if (Array.isArray(pathsField)) { for (const p of pathsField) push(p); } - if (toolName === ToolNames.GLOB) { + if (canonical === ToolNames.GLOB) { // Glob-only: derive the effective selector from `path` + `pattern`. // The previous version pushed `pattern` standalone, but the glob // call actually searches `/` — so a skill keyed on @@ -1845,11 +1852,21 @@ export class CoreToolScheduler { const activatedSkills = await skillManager?.matchAndActivateByPaths(candidatePaths); if (activatedSkills && activatedSkills.length > 0) { - const names = activatedSkills.join(', '); - content = appendAdditionalContext( - content, - `\nThe following skill(s) are now available via the Skill tool based on the file you just accessed: ${names}. Use them if relevant to the task.\n`, - ); + // Subagents share the parent's SkillManager but may have a + // restricted toolsList that excludes SkillTool entirely. + // Telling such a context "skill X is now available via the + // Skill tool" is misleading — the subagent can't invoke it + // and would waste a turn trying. Gate the reminder on + // whether the active tool registry actually exposes + // SkillTool to the model. + const hasSkillTool = !!this.toolRegistry.getTool(ToolNames.SKILL); + if (hasSkillTool) { + const names = activatedSkills.join(', '); + content = appendAdditionalContext( + content, + `\nThe following skill(s) are now available via the Skill tool based on the file you just accessed: ${names}. Use them if relevant to the task.\n`, + ); + } } } diff --git a/packages/core/src/core/toolHookTriggers.test.ts b/packages/core/src/core/toolHookTriggers.test.ts index 1e93fceb4..57d6539cd 100644 --- a/packages/core/src/core/toolHookTriggers.test.ts +++ b/packages/core/src/core/toolHookTriggers.test.ts @@ -460,14 +460,22 @@ describe('toolHookTriggers', () => { ]); }); - it('should handle non-array PartListUnion content', () => { - const originalContent = { text: 'original' }; + it('wraps single non-array PartListUnion content so the addition still lands', () => { + // Regression: `ReadFile` returns `{ inlineData: {...} }` for images + // and PDFs (a single Part, not an array). The previous "return + // content unchanged" silently dropped any hook-injected reminder + // — including the path-conditional skill activation `` + // and the ConditionalRulesRegistry rule injection. Wrap into an array + // so the additional context lands. + const originalContent = { + inlineData: { data: 'base64', mimeType: 'image/png' }, + }; const result = appendAdditionalContext( originalContent, 'additional context', ); - expect(result).toEqual({ text: 'original' }); + expect(result).toEqual([originalContent, { text: 'additional context' }]); }); it('should return original array content when no additional context is provided', () => { diff --git a/packages/core/src/core/toolHookTriggers.ts b/packages/core/src/core/toolHookTriggers.ts index 73423d77d..65c9eb38a 100644 --- a/packages/core/src/core/toolHookTriggers.ts +++ b/packages/core/src/core/toolHookTriggers.ts @@ -483,6 +483,9 @@ export function appendAdditionalContext( return [...content, { text: additionalContext } as Part]; } - // For non-array content that's still PartListUnion, return as-is - return content; + // Single non-array Part (e.g. ReadFile returning `{ inlineData: {...} }` + // for an image or PDF). Wrap in an array so the additional context still + // reaches the model — the previous "return as-is" silently dropped + // hook-injected reminders for any tool whose llmContent is a single Part. + return [content, { text: additionalContext } as Part]; } diff --git a/packages/core/src/extension/extensionManager.ts b/packages/core/src/extension/extensionManager.ts index 60f714a32..c79f724fe 100644 --- a/packages/core/src/extension/extensionManager.ts +++ b/packages/core/src/extension/extensionManager.ts @@ -1347,18 +1347,31 @@ export class ExtensionManager { // refresh mcp servers await this.config.getToolRegistry().restartMcpServers(); // Refresh skills + subagents in parallel. Both `refreshCache` calls - // now resolve only after their async change-listener chain settles — - // for skills, that includes `SkillTool.refreshSkills()` rebuilding + // now resolve only after their async change-listener chain settles + // — for skills, that includes `SkillTool.refreshSkills()` rebuilding // the model-facing tool description and updating `geminiClient`'s - // tool list. Without these awaits the function would resolve before - // those side-effects land, and any rejection from them would be - // silently swallowed. - await Promise.all([ - this.config.getSkillManager()?.refreshCache(), + // tool list. allSettled (rather than Promise.all) so a rejection + // from one leg does not cascade — the other leg's result is still + // applied, refreshHierarchicalMemory below still runs, and the + // `refreshTools` callers (`enableExtension`, etc.) don't unwind + // because of an unrelated transient failure. + const skillManager = this.config.getSkillManager(); + const settled = await Promise.allSettled([ + skillManager?.refreshCache(), this.config.getSubagentManager().refreshCache(), ]); - // refresh context files - this.config.refreshHierarchicalMemory(); + for (const result of settled) { + if (result.status === 'rejected') { + debugLogger.warn( + 'refreshMemory: a refreshCache leg failed:', + result.reason, + ); + } + } + // Hierarchical memory refresh is now awaited too — the previous + // fire-and-forget defeated the rest of the function's "wait until + // refresh is done" contract. + await this.config.refreshHierarchicalMemory(); } async refreshTools(): Promise { diff --git a/packages/core/src/skills/skill-activation.test.ts b/packages/core/src/skills/skill-activation.test.ts index 899ca78cf..b2a6f6b9d 100644 --- a/packages/core/src/skills/skill-activation.test.ts +++ b/packages/core/src/skills/skill-activation.test.ts @@ -133,6 +133,30 @@ describe('SkillActivationRegistry', () => { expect(reg.matchAndConsume('/project/src/Bar.tsx')).toEqual([]); }); + it('survives an invalid picomatch pattern (drops it, keeps the rest)', () => { + // Regression: picomatch can throw on pathological patterns + // (oversized strings, broken extglob nesting). The constructor + // previously let that throw escape and abort skill loading + // entirely. With the try/catch, the bad pattern is dropped with + // a debug log and the remaining patterns still compile. + // + // Use an oversized pattern (~70 KB) — picomatch's default limit + // is 65,536 chars and it throws above that. + const bigPattern = 'a'.repeat(70_000); + const reg = new SkillActivationRegistry( + [ + makeSkill({ + name: 'mixed', + paths: [bigPattern, 'src/**/*.ts'], + }), + ], + projectRoot, + ); + expect(reg.totalCount).toBe(1); + // The good pattern still works. + expect(reg.matchAndConsume('/project/src/App.ts')).toEqual(['mixed']); + }); + it('rejects an absolute relative path (Windows cross-drive case)', () => { // Regression: on Windows, `path.relative('C:\\project', 'D:\\other')` // returns an absolute path like `D:\\other`. After normalizing diff --git a/packages/core/src/skills/skill-activation.ts b/packages/core/src/skills/skill-activation.ts index b0a809a3a..97d2ec929 100644 --- a/packages/core/src/skills/skill-activation.ts +++ b/packages/core/src/skills/skill-activation.ts @@ -94,10 +94,24 @@ export class SkillActivationRegistry { constructor(conditionalSkills: readonly SkillConfig[], projectRoot: string) { this.projectRoot = projectRoot; - this.compiled = conditionalSkills.map((skill) => ({ - skill, - matchers: (skill.paths ?? []).map((p) => picomatch(p, { dot: false })), - })); + this.compiled = conditionalSkills.map((skill) => { + const matchers: picomatch.Matcher[] = []; + for (const p of skill.paths ?? []) { + try { + matchers.push(picomatch(p, { dot: false })); + } catch (e) { + // picomatch can throw on pathological inputs (oversize patterns, + // broken extglob nesting). Drop the offending pattern but keep + // the rest of the skill — better than letting the error bubble + // up to refreshCache and abort skill loading entirely (this + // site is outside the levels-level Promise.allSettled boundary). + debugLogger.warn( + `Skill "${skill.name}" has invalid glob "${p}", skipping pattern: ${e instanceof Error ? e.message : String(e)}`, + ); + } + } + return { skill, matchers }; + }); } /** diff --git a/packages/core/src/skills/skill-load.test.ts b/packages/core/src/skills/skill-load.test.ts index f7effa9fe..9f7700506 100644 --- a/packages/core/src/skills/skill-load.test.ts +++ b/packages/core/src/skills/skill-load.test.ts @@ -439,6 +439,83 @@ Valid skill. expect(() => validateSkillName('newline\nin-name')).toThrow(); expect(() => validateSkillName('quote"in-name')).toThrow(); }); + + it('accepts non-ASCII letters (CJK / Cyrillic / accented Latin)', async () => { + const { validateSkillName } = await import('./types.js'); + // Regression: the previous /^[a-zA-Z0-9_:.-]+$/ rejected every + // non-ASCII name, silently dropping CJK skills on upgrade. The + // structural-injection guard targets <>"'/\\\n\r\t etc — entire + // Unicode planes are not the threat. + expect(() => validateSkillName('中文助手')).not.toThrow(); + expect(() => validateSkillName('помощник')).not.toThrow(); + expect(() => validateSkillName('café-helper')).not.toThrow(); + expect(() => validateSkillName('日本語_v2')).not.toThrow(); + }); + }); + + describe('parsePathsField content validation', () => { + it('rejects absolute path entries (project-relative only)', async () => { + const { parsePathsField } = await import('./types.js'); + expect(() => parsePathsField({ paths: ['/etc/passwd'] })).toThrow( + /looks absolute/, + ); + expect(() => parsePathsField({ paths: ['\\\\server\\share'] })).toThrow( + /looks absolute/, + ); + }); + + it('rejects parent-dir-escape patterns', async () => { + const { parsePathsField } = await import('./types.js'); + expect(() => parsePathsField({ paths: ['../*.ts'] })).toThrow( + /escapes the project root/, + ); + expect(() => parsePathsField({ paths: ['..'] })).toThrow( + /escapes the project root/, + ); + }); + + it('still accepts in-project relative globs', async () => { + const { parsePathsField } = await import('./types.js'); + expect(parsePathsField({ paths: ['src/**/*.ts', '**/*.tsx'] })).toEqual([ + 'src/**/*.ts', + '**/*.tsx', + ]); + }); + }); + + describe('extension parser parity (skill-load.ts)', () => { + it('extracts disable-model-invocation alongside paths', () => { + // Regression: the extension parser previously dropped the + // disable-model-invocation field, so an extension SKILL.md with + // both `paths:` and `disable-model-invocation: true` would still + // be eligible for path activation — directly contradicting the + // bug_004 fix at the project/user level. + mockParseYaml.mockReturnValueOnce({ + name: 'secret-helper', + description: 'Hidden helper', + paths: ['src/**/*.ts'], + 'disable-model-invocation': true, + }); + const config = parseSkillContent( + `---\nname: secret-helper\ndescription: Hidden helper\npaths:\n - "src/**/*.ts"\ndisable-model-invocation: true\n---\n\nBody.\n`, + '/test/extension/skills/secret-helper/SKILL.md', + ); + expect(config.disableModelInvocation).toBe(true); + expect(config.paths).toEqual(['src/**/*.ts']); + }); + + it('extracts when_to_use', () => { + mockParseYaml.mockReturnValueOnce({ + name: 'tsx-helper', + description: 'React skill', + when_to_use: 'When editing React components', + }); + const config = parseSkillContent( + `---\nname: tsx-helper\ndescription: React skill\nwhen_to_use: When editing React components\n---\n\nBody.\n`, + '/test/extension/skills/tsx-helper/SKILL.md', + ); + expect(config.whenToUse).toBe('When editing React components'); + }); }); describe('parseSkillContent model field', () => { diff --git a/packages/core/src/skills/skill-load.ts b/packages/core/src/skills/skill-load.ts index 49eca16fd..8b031e186 100644 --- a/packages/core/src/skills/skill-load.ts +++ b/packages/core/src/skills/skill-load.ts @@ -125,6 +125,22 @@ export function parseSkillContent( ? frontmatter['argument-hint'] : undefined; + // `whenToUse` and `disable-model-invocation` were historically only + // parsed by the project/user/bundled parser in skill-manager.ts, which + // meant an extension SKILL.md with `disable-model-invocation: true` + // had the flag silently stripped — and (post-paths PR) would still + // fire path-activation reminders for a skill the model can't invoke. + // Extract them here too so the extension and managed parsers agree. + const whenToUse = + typeof frontmatter['when_to_use'] === 'string' + ? frontmatter['when_to_use'] + : undefined; + const disableModelInvocationRaw = frontmatter['disable-model-invocation']; + const disableModelInvocation = + disableModelInvocationRaw === true || disableModelInvocationRaw === 'true' + ? true + : undefined; + // Optional `paths` frontmatter: glob patterns that gate when this skill // is offered to the model (conditional skill). const paths = parsePathsField(frontmatter); @@ -138,6 +154,8 @@ export function parseSkillContent( filePath, body: body.trim(), level: 'extension', + whenToUse, + disableModelInvocation, paths, }; diff --git a/packages/core/src/skills/types.ts b/packages/core/src/skills/types.ts index e0066314b..5a8aed765 100644 --- a/packages/core/src/skills/types.ts +++ b/packages/core/src/skills/types.ts @@ -163,17 +163,44 @@ export function parsePathsField( throw new Error('"paths" must be an array of glob patterns'); } const cleaned = raw.map((p) => String(p).trim()).filter((p) => p.length > 0); + // Surface obvious silent-failures at parse time. Without these the + // skill loads fine but its globs never match anything: the activation + // registry resolves candidates against the project root and rejects + // anything starting with `..` or anything that resolves outside the + // project. An author who writes `paths: ['/etc/passwd']` or `['../*.ts']` + // would otherwise see the skill in `/skills` and never understand why + // it never activates from the model side. + for (const pattern of cleaned) { + if (pattern.startsWith('/') || pattern.startsWith('\\')) { + throw new Error( + `"paths" entry "${pattern}" looks absolute; patterns are project-root-relative — drop the leading slash`, + ); + } + if (pattern === '..' || pattern.startsWith('../')) { + throw new Error( + `"paths" entry "${pattern}" escapes the project root; patterns must stay within the project`, + ); + } + } return cleaned.length > 0 ? cleaned : undefined; } /** - * Strict allowed-character set for skill names. Names flow into multiple + * Allowed-character set for skill names. Names flow into multiple * trust-relevant sinks: the `` description (consumed by * the model), the `` emitted on path activation, the * SkillTool schema's enum, and various UI listings. Rejecting structurally - * unsafe names at parse time is more reliable than escaping at every sink. + * unsafe characters at parse time is more reliable than escaping at every + * sink. + * + * Charset uses Unicode property classes so non-ASCII names (CJK, Cyrillic, + * accented Latin, etc.) keep working — the original `[a-zA-Z0-9_:.-]+` + * silently dropped any such skill on upgrade, which is a real + * backwards-compat regression for the project's CJK userbase. The + * structurally unsafe characters (`<>/\"'\n\r\t`, whitespace) are still + * out, which is the actual injection vector this guards against. */ -export const SKILL_NAME_PATTERN = /^[a-zA-Z0-9_:.-]+$/; +export const SKILL_NAME_PATTERN = /^[\p{L}\p{N}_:.-]+$/u; /** * Validate that a skill `name` is safe to embed into prompts and reminders diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index a660363c9..5a7e915df 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -351,6 +351,20 @@ describe('SkillTool', () => { }); }); + describe('dispose', () => { + it('detaches the change listener so per-subagent SkillTools do not leak', () => { + // Regression: subagents share the parent's SkillManager via + // InProcessBackend.createPerAgentConfig, so each per-subagent + // SkillTool registers its own listener on the parent's manager. + // Without dispose() the listeners accumulate and every + // matchAndActivateByPaths call awaits each stale subagent's + // refreshSkills sequentially. + expect(changeListeners.length).toBe(1); + (skillTool as unknown as { dispose: () => void }).dispose(); + expect(changeListeners.length).toBe(0); + }); + }); + describe('SkillToolInvocation', () => { const mockRuntimeConfig: SkillConfig = { ...mockSkills[0], diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index c1aa486ca..c721ffe54 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -46,6 +46,13 @@ export class SkillTool extends BaseDeclarativeTool { description: string; }> = []; private loadedSkillNames: Set = new Set(); + // Cleanup function returned by `addChangeListener`. Stored so per-agent + // SkillTool instances (subagents share the parent's SkillManager) can + // detach their listener at teardown — without this the SkillManager + // accumulates listeners across subagent lifetimes, and each path + // activation would serialize through every stale listener's + // refreshSkills / setTools round-trip. + private removeChangeListener: () => void; constructor(private readonly config: Config) { // Initialize with a basic schema first @@ -82,7 +89,9 @@ export class SkillTool extends BaseDeclarativeTool { // tool description picks up the newly activated skill, and the // announcing the activation can land in the same // turn as a still-stale listing. - this.skillManager.addChangeListener(() => this.refreshSkills()); + this.removeChangeListener = this.skillManager.addChangeListener(() => + this.refreshSkills(), + ); // Initialize the tool asynchronously this.refreshSkills(); @@ -295,6 +304,20 @@ ${skillDescriptions} clearLoadedSkills(): void { this.loadedSkillNames.clear(); } + + /** + * Detach the change listener from SkillManager. Tool registries call + * this on teardown (mirroring AgentTool's pattern). Per-subagent + * SkillTool instances share the parent's SkillManager via + * `InProcessBackend.createPerAgentConfig`, so without dispose the + * SkillManager would accumulate one stale listener per subagent + * lifetime — and `notifyChangeListeners` is now `await`-ed + * sequentially, so each path activation would serialize through every + * accumulated listener's refreshSkills + setTools round-trip. + */ + dispose(): void { + this.removeChangeListener(); + } } class SkillToolInvocation extends BaseToolInvocation {