mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-04-28 11:41:04 +00:00
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.
This commit is contained in:
parent
99b9fe4de2
commit
5432f5ddc8
10 changed files with 636 additions and 8 deletions
|
|
@ -1713,6 +1713,21 @@ export class CoreToolScheduler {
|
|||
`<system-reminder>\n${rulesCtx}\n</system-reminder>`,
|
||||
);
|
||||
}
|
||||
|
||||
// Activate any conditional skills (skills with `paths:` frontmatter)
|
||||
// whose globs match this file. Newly activated skills appear in the
|
||||
// SkillTool listing on the next turn; announce them to the model via
|
||||
// a system-reminder so it knows they became available.
|
||||
const activatedSkills = this.config
|
||||
.getSkillManager()
|
||||
?.matchAndActivateByPath(filePath);
|
||||
if (activatedSkills && activatedSkills.length > 0) {
|
||||
const names = activatedSkills.join(', ');
|
||||
content = appendAdditionalContext(
|
||||
content,
|
||||
`<system-reminder>\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</system-reminder>`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
const response = convertToFunctionResponse(toolName, callId, content);
|
||||
|
|
|
|||
|
|
@ -33,3 +33,9 @@ export { SkillError } from './types.js';
|
|||
|
||||
// Main management class
|
||||
export { SkillManager } from './skill-manager.js';
|
||||
|
||||
// Path-based conditional skill activation
|
||||
export {
|
||||
SkillActivationRegistry,
|
||||
splitConditionalSkills,
|
||||
} from './skill-activation.js';
|
||||
|
|
|
|||
133
packages/core/src/skills/skill-activation.test.ts
Normal file
133
packages/core/src/skills/skill-activation.test.ts
Normal file
|
|
@ -0,0 +1,133 @@
|
|||
/**
|
||||
* @license
|
||||
* Copyright 2025 Qwen
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import {
|
||||
SkillActivationRegistry,
|
||||
splitConditionalSkills,
|
||||
} from './skill-activation.js';
|
||||
import type { SkillConfig } from './types.js';
|
||||
|
||||
function makeSkill(overrides: Partial<SkillConfig>): SkillConfig {
|
||||
return {
|
||||
name: overrides.name ?? 'test-skill',
|
||||
description: overrides.description ?? 'desc',
|
||||
body: overrides.body ?? '',
|
||||
level: overrides.level ?? 'project',
|
||||
filePath: overrides.filePath ?? '/proj/.qwen/skills/test/SKILL.md',
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
describe('splitConditionalSkills', () => {
|
||||
it('treats skills without paths as unconditional', () => {
|
||||
const skills = [makeSkill({ name: 'a' })];
|
||||
const { unconditional, conditional } = splitConditionalSkills(skills);
|
||||
expect(unconditional).toHaveLength(1);
|
||||
expect(conditional).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('treats empty paths array as unconditional', () => {
|
||||
const skills = [makeSkill({ name: 'a', paths: [] })];
|
||||
const { unconditional, conditional } = splitConditionalSkills(skills);
|
||||
expect(unconditional).toHaveLength(1);
|
||||
expect(conditional).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('classifies skills with non-empty paths as conditional', () => {
|
||||
const skills = [
|
||||
makeSkill({ name: 'a' }),
|
||||
makeSkill({ name: 'b', paths: ['src/**/*.tsx'] }),
|
||||
];
|
||||
const { unconditional, conditional } = splitConditionalSkills(skills);
|
||||
expect(unconditional.map((s) => s.name)).toEqual(['a']);
|
||||
expect(conditional.map((s) => s.name)).toEqual(['b']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('SkillActivationRegistry', () => {
|
||||
const projectRoot = '/project';
|
||||
|
||||
it('returns empty when no conditional skills are registered', () => {
|
||||
const reg = new SkillActivationRegistry([], projectRoot);
|
||||
expect(reg.matchAndConsume('/project/src/App.tsx')).toEqual([]);
|
||||
expect(reg.totalCount).toBe(0);
|
||||
});
|
||||
|
||||
it('activates a conditional skill when a matching path is touched', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
|
||||
projectRoot,
|
||||
);
|
||||
const newly = reg.matchAndConsume('/project/src/App.tsx');
|
||||
expect(newly).toEqual(['tsx-helper']);
|
||||
expect(reg.isActivated('tsx-helper')).toBe(true);
|
||||
expect(reg.activatedCount).toBe(1);
|
||||
});
|
||||
|
||||
it('does not re-activate an already-active skill on subsequent matches', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
|
||||
projectRoot,
|
||||
);
|
||||
expect(reg.matchAndConsume('/project/src/A.tsx')).toEqual(['tsx-helper']);
|
||||
// Second touch of the same pattern returns nothing new.
|
||||
expect(reg.matchAndConsume('/project/src/B.tsx')).toEqual([]);
|
||||
expect(reg.activatedCount).toBe(1);
|
||||
});
|
||||
|
||||
it('returns empty for paths that do not match any skill', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
|
||||
projectRoot,
|
||||
);
|
||||
expect(reg.matchAndConsume('/project/lib/utils.py')).toEqual([]);
|
||||
});
|
||||
|
||||
it('activates multiple skills whose globs overlap on a single file', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[
|
||||
makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] }),
|
||||
makeSkill({ name: 'app-helper', paths: ['src/App.tsx'] }),
|
||||
],
|
||||
projectRoot,
|
||||
);
|
||||
const newly = reg.matchAndConsume('/project/src/App.tsx');
|
||||
expect(newly.sort()).toEqual(['app-helper', 'tsx-helper']);
|
||||
});
|
||||
|
||||
it('accepts relative file paths by resolving against the project root', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
|
||||
projectRoot,
|
||||
);
|
||||
expect(reg.matchAndConsume('src/App.tsx')).toEqual(['tsx-helper']);
|
||||
});
|
||||
|
||||
it('ignores paths outside the project root', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
|
||||
projectRoot,
|
||||
);
|
||||
expect(reg.matchAndConsume('/other/project/src/App.tsx')).toEqual([]);
|
||||
expect(reg.activatedCount).toBe(0);
|
||||
});
|
||||
|
||||
it('supports multiple glob patterns per skill (OR semantics)', () => {
|
||||
const reg = new SkillActivationRegistry(
|
||||
[
|
||||
makeSkill({
|
||||
name: 'multi',
|
||||
paths: ['src/**/*.tsx', 'test/**/*.ts'],
|
||||
}),
|
||||
],
|
||||
projectRoot,
|
||||
);
|
||||
// Both patterns should activate the same skill, but only once total.
|
||||
expect(reg.matchAndConsume('/project/test/foo.ts')).toEqual(['multi']);
|
||||
expect(reg.matchAndConsume('/project/src/Bar.tsx')).toEqual([]);
|
||||
});
|
||||
});
|
||||
118
packages/core/src/skills/skill-activation.ts
Normal file
118
packages/core/src/skills/skill-activation.ts
Normal file
|
|
@ -0,0 +1,118 @@
|
|||
/**
|
||||
* @license
|
||||
* Copyright 2025 Qwen
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
// Path-based skill activation (turn-level lazy offering).
|
||||
//
|
||||
// Skills with a `paths:` frontmatter are "conditional": they stay out of the
|
||||
// SkillTool listing until a tool call touches a file matching one of their
|
||||
// glob patterns. This keeps the model's tool description small in large
|
||||
// monorepos where most skills are irrelevant to the current task.
|
||||
//
|
||||
// Mirrors the design of ConditionalRulesRegistry in utils/rulesDiscovery.ts
|
||||
// but returns skill names (not content), because the activation affects which
|
||||
// skills are advertised in SkillTool's description rather than injecting text.
|
||||
|
||||
import * as path from 'node:path';
|
||||
import picomatch from 'picomatch';
|
||||
import type { SkillConfig } from './types.js';
|
||||
|
||||
interface CompiledSkill {
|
||||
readonly skill: SkillConfig;
|
||||
readonly matchers: picomatch.Matcher[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Splits a skill list into unconditional skills (no `paths:`) and conditional
|
||||
* skills (with non-empty `paths:`). Unconditional skills are always offered to
|
||||
* the model; conditional skills only appear after activation.
|
||||
*/
|
||||
export function splitConditionalSkills(skills: readonly SkillConfig[]): {
|
||||
unconditional: SkillConfig[];
|
||||
conditional: SkillConfig[];
|
||||
} {
|
||||
const unconditional: SkillConfig[] = [];
|
||||
const conditional: SkillConfig[] = [];
|
||||
for (const skill of skills) {
|
||||
if (skill.paths && skill.paths.length > 0) {
|
||||
conditional.push(skill);
|
||||
} else {
|
||||
unconditional.push(skill);
|
||||
}
|
||||
}
|
||||
return { unconditional, conditional };
|
||||
}
|
||||
|
||||
/**
|
||||
* Tracks which conditional skills have been activated during the session by
|
||||
* matching tool-invocation file paths against each skill's `paths` globs.
|
||||
*
|
||||
* Once activated, a skill stays active for the rest of the registry's
|
||||
* lifetime. A new registry is constructed on every `refreshCache()` so that
|
||||
* edits to skill files (adding/removing `paths`) take effect; prior
|
||||
* activations do not carry over across rebuilds (same as
|
||||
* ConditionalRulesRegistry).
|
||||
*/
|
||||
export class SkillActivationRegistry {
|
||||
private readonly compiled: CompiledSkill[];
|
||||
private readonly activated = new Set<string>();
|
||||
private readonly projectRoot: string;
|
||||
|
||||
constructor(conditionalSkills: readonly SkillConfig[], projectRoot: string) {
|
||||
this.projectRoot = projectRoot;
|
||||
this.compiled = conditionalSkills.map((skill) => ({
|
||||
skill,
|
||||
matchers: (skill.paths ?? []).map((p) => picomatch(p, { dot: false })),
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* Activate any conditional skills whose `paths` globs match `filePath`.
|
||||
* Returns the names of skills newly activated by this call (empty when
|
||||
* either no skill matched, or every match was already active).
|
||||
*/
|
||||
matchAndConsume(filePath: string): string[] {
|
||||
if (this.compiled.length === 0) return [];
|
||||
|
||||
const absolutePath = path.isAbsolute(filePath)
|
||||
? filePath
|
||||
: path.resolve(this.projectRoot, filePath);
|
||||
const relativePath = path
|
||||
.relative(this.projectRoot, absolutePath)
|
||||
.replace(/\\/g, '/');
|
||||
|
||||
// Skip files outside the project root — conditional skills are scoped to
|
||||
// the project, matching ConditionalRulesRegistry's behavior.
|
||||
if (relativePath === '..' || relativePath.startsWith('../')) {
|
||||
return [];
|
||||
}
|
||||
|
||||
const newlyActivated: string[] = [];
|
||||
for (const { skill, matchers } of this.compiled) {
|
||||
if (this.activated.has(skill.name)) continue;
|
||||
if (matchers.some((m) => m(relativePath))) {
|
||||
this.activated.add(skill.name);
|
||||
newlyActivated.push(skill.name);
|
||||
}
|
||||
}
|
||||
return newlyActivated;
|
||||
}
|
||||
|
||||
isActivated(name: string): boolean {
|
||||
return this.activated.has(name);
|
||||
}
|
||||
|
||||
getActivatedNames(): ReadonlySet<string> {
|
||||
return this.activated;
|
||||
}
|
||||
|
||||
get totalCount(): number {
|
||||
return this.compiled.length;
|
||||
}
|
||||
|
||||
get activatedCount(): number {
|
||||
return this.activated.size;
|
||||
}
|
||||
}
|
||||
|
|
@ -115,6 +115,20 @@ export function parseSkillContent(
|
|||
// Extract optional model field
|
||||
const model = parseModelField(frontmatter);
|
||||
|
||||
// Optional `paths` frontmatter: glob patterns that gate when this skill
|
||||
// is offered to the model (conditional skill).
|
||||
const pathsRaw = frontmatter['paths'];
|
||||
let paths: string[] | undefined;
|
||||
if (pathsRaw !== undefined) {
|
||||
if (!Array.isArray(pathsRaw)) {
|
||||
throw new Error('"paths" must be an array of glob patterns');
|
||||
}
|
||||
const cleaned = pathsRaw
|
||||
.map((p) => String(p).trim())
|
||||
.filter((p) => p.length > 0);
|
||||
paths = cleaned.length > 0 ? cleaned : undefined;
|
||||
}
|
||||
|
||||
const config: SkillConfig = {
|
||||
name,
|
||||
description,
|
||||
|
|
@ -123,6 +137,7 @@ export function parseSkillContent(
|
|||
filePath,
|
||||
body: body.trim(),
|
||||
level: 'extension',
|
||||
paths,
|
||||
};
|
||||
|
||||
// Validate the parsed configuration
|
||||
|
|
|
|||
|
|
@ -82,6 +82,29 @@ describe('SkillManager', () => {
|
|||
allowedTools: ['read_file', 'write_file'],
|
||||
};
|
||||
}
|
||||
if (yamlString.includes('paths:')) {
|
||||
// Branch handles all three paths-related tests by reading the literal
|
||||
// YAML so the parser-behavior nuance (array vs scalar vs empty) is
|
||||
// preserved.
|
||||
const name = yamlString.includes('name: tsx-helper')
|
||||
? 'tsx-helper'
|
||||
: 'test-skill';
|
||||
const description = yamlString.includes('React skill')
|
||||
? 'React skill'
|
||||
: 'A test skill';
|
||||
let paths: unknown = undefined;
|
||||
if (yamlString.includes('paths: []')) {
|
||||
paths = [];
|
||||
} else if (yamlString.includes('paths: "src/**/*.tsx"')) {
|
||||
// Invalid (scalar) — surface as string so our validator rejects it.
|
||||
paths = 'src/**/*.tsx';
|
||||
} else if (yamlString.includes('src/**/*.tsx')) {
|
||||
paths = yamlString.includes('test/**/*.tsx')
|
||||
? ['src/**/*.tsx', 'test/**/*.tsx']
|
||||
: ['src/**/*.tsx'];
|
||||
}
|
||||
return { name, description, paths };
|
||||
}
|
||||
if (yamlString.includes('name: skill1')) {
|
||||
return { name: 'skill1', description: 'First skill' };
|
||||
}
|
||||
|
|
@ -239,6 +262,76 @@ You are a helpful assistant with this skill.
|
|||
expect(config.allowedTools).toEqual(['read_file', 'write_file']);
|
||||
});
|
||||
|
||||
it('should parse content with paths (conditional skill)', () => {
|
||||
const markdown = `---
|
||||
name: tsx-helper
|
||||
description: React skill
|
||||
paths:
|
||||
- "src/**/*.tsx"
|
||||
- "test/**/*.tsx"
|
||||
---
|
||||
|
||||
Body.
|
||||
`;
|
||||
const config = manager.parseSkillContent(
|
||||
markdown,
|
||||
validSkillConfig.filePath,
|
||||
'project',
|
||||
);
|
||||
expect(config.paths).toEqual(['src/**/*.tsx', 'test/**/*.tsx']);
|
||||
});
|
||||
|
||||
it('should leave paths undefined when frontmatter omits it', () => {
|
||||
const markdown = `---
|
||||
name: test-skill
|
||||
description: A test skill
|
||||
---
|
||||
|
||||
Body.
|
||||
`;
|
||||
const config = manager.parseSkillContent(
|
||||
markdown,
|
||||
validSkillConfig.filePath,
|
||||
'project',
|
||||
);
|
||||
expect(config.paths).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should treat an empty paths array as undefined (unconditional)', () => {
|
||||
const markdown = `---
|
||||
name: test-skill
|
||||
description: A test skill
|
||||
paths: []
|
||||
---
|
||||
|
||||
Body.
|
||||
`;
|
||||
const config = manager.parseSkillContent(
|
||||
markdown,
|
||||
validSkillConfig.filePath,
|
||||
'project',
|
||||
);
|
||||
expect(config.paths).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should throw when paths is not an array', () => {
|
||||
const markdown = `---
|
||||
name: test-skill
|
||||
description: A test skill
|
||||
paths: "src/**/*.tsx"
|
||||
---
|
||||
|
||||
Body.
|
||||
`;
|
||||
expect(() =>
|
||||
manager.parseSkillContent(
|
||||
markdown,
|
||||
validSkillConfig.filePath,
|
||||
'project',
|
||||
),
|
||||
).toThrow(/"paths" must be an array/);
|
||||
});
|
||||
|
||||
it('should determine level from file path', () => {
|
||||
const projectPath = '/test/project/.qwen/skills/test-skill/SKILL.md';
|
||||
const userPath = '/home/user/.qwen/skills/test-skill/SKILL.md';
|
||||
|
|
@ -793,6 +886,81 @@ Review content`);
|
|||
});
|
||||
});
|
||||
|
||||
describe('conditional skill activation', () => {
|
||||
// Minimal setup: a project dir containing one conditional skill whose
|
||||
// paths glob matches `src/**/*.tsx`. After refreshCache() loads it,
|
||||
// matchAndActivateByPath() should activate it and fire listeners.
|
||||
async function loadConditionalFixture() {
|
||||
vi.mocked(fs.readdir).mockResolvedValue([
|
||||
{
|
||||
name: 'tsx-helper',
|
||||
isDirectory: () => true,
|
||||
isFile: () => false,
|
||||
isSymbolicLink: () => false,
|
||||
},
|
||||
] as unknown as Awaited<ReturnType<typeof fs.readdir>>);
|
||||
vi.mocked(fs.access).mockResolvedValue(undefined);
|
||||
vi.mocked(fs.readFile).mockResolvedValue(`---
|
||||
name: tsx-helper
|
||||
description: React skill
|
||||
paths:
|
||||
- "src/**/*.tsx"
|
||||
---
|
||||
|
||||
Body.
|
||||
`);
|
||||
await manager.refreshCache();
|
||||
}
|
||||
|
||||
it('keeps conditional skills inactive until a matching path is touched', async () => {
|
||||
await loadConditionalFixture();
|
||||
|
||||
const all = await manager.listSkills();
|
||||
const tsx = all.find((s) => s.name === 'tsx-helper');
|
||||
expect(tsx).toBeDefined();
|
||||
expect(manager.isSkillActive(tsx!)).toBe(false);
|
||||
});
|
||||
|
||||
it('activates a conditional skill when a matching file path is touched', async () => {
|
||||
await loadConditionalFixture();
|
||||
|
||||
const newly = manager.matchAndActivateByPath('/test/project/src/App.tsx');
|
||||
expect(newly).toEqual(['tsx-helper']);
|
||||
expect(manager.getActivatedSkillNames().has('tsx-helper')).toBe(true);
|
||||
|
||||
const all = await manager.listSkills();
|
||||
const tsx = all.find((s) => s.name === 'tsx-helper')!;
|
||||
expect(manager.isSkillActive(tsx)).toBe(true);
|
||||
});
|
||||
|
||||
it('does not re-notify listeners on subsequent matches of the same skill', async () => {
|
||||
await loadConditionalFixture();
|
||||
|
||||
const listener = vi.fn();
|
||||
manager.addChangeListener(listener);
|
||||
|
||||
expect(manager.matchAndActivateByPath('/test/project/src/A.tsx')).toEqual(
|
||||
['tsx-helper'],
|
||||
);
|
||||
expect(listener).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Same pattern touched again — skill already active, no new
|
||||
// notification.
|
||||
expect(manager.matchAndActivateByPath('/test/project/src/B.tsx')).toEqual(
|
||||
[],
|
||||
);
|
||||
expect(listener).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('does nothing for paths outside the project root', async () => {
|
||||
await loadConditionalFixture();
|
||||
expect(manager.matchAndActivateByPath('/other/place/foo.tsx')).toEqual(
|
||||
[],
|
||||
);
|
||||
expect(manager.getActivatedSkillNames().size).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('parse errors', () => {
|
||||
it('should track parse errors', async () => {
|
||||
vi.mocked(fs.readdir).mockResolvedValue([
|
||||
|
|
|
|||
|
|
@ -22,6 +22,10 @@ import type {
|
|||
import { SkillError, SkillErrorCode, parseModelField } from './types.js';
|
||||
import type { Config } from '../config/config.js';
|
||||
import { validateConfig } from './skill-load.js';
|
||||
import {
|
||||
SkillActivationRegistry,
|
||||
splitConditionalSkills,
|
||||
} from './skill-activation.js';
|
||||
import { createDebugLogger } from '../utils/debugLogger.js';
|
||||
import { normalizeContent } from '../utils/textUtils.js';
|
||||
import { SKILL_PROVIDER_CONFIG_DIRS } from '../config/storage.js';
|
||||
|
|
@ -66,6 +70,7 @@ export class SkillManager {
|
|||
private watchStarted = false;
|
||||
private refreshTimer: NodeJS.Timeout | null = null;
|
||||
private readonly bundledSkillsDir: string;
|
||||
private activationRegistry: SkillActivationRegistry | null = null;
|
||||
|
||||
constructor(private readonly config: Config) {
|
||||
this.bundledSkillsDir = path.join(
|
||||
|
|
@ -278,18 +283,61 @@ export class SkillManager {
|
|||
);
|
||||
|
||||
let totalSkills = 0;
|
||||
const allSkills: SkillConfig[] = [];
|
||||
for (const [level, levelSkills] of loaded) {
|
||||
skillsCache.set(level, levelSkills);
|
||||
totalSkills += levelSkills.length;
|
||||
allSkills.push(...levelSkills);
|
||||
}
|
||||
|
||||
this.skillsCache = skillsCache;
|
||||
|
||||
// Rebuild the activation registry so that newly added/removed `paths:`
|
||||
// frontmatter takes effect. Prior activations do not carry across reloads.
|
||||
const { conditional } = splitConditionalSkills(allSkills);
|
||||
this.activationRegistry = new SkillActivationRegistry(
|
||||
conditional,
|
||||
this.config.getProjectRoot(),
|
||||
);
|
||||
|
||||
debugLogger.info(
|
||||
`Skills cache refreshed: ${totalSkills} total skills loaded`,
|
||||
`Skills cache refreshed: ${totalSkills} total skills loaded ` +
|
||||
`(${conditional.length} conditional)`,
|
||||
);
|
||||
this.notifyChangeListeners();
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the given skill is currently eligible to appear in the SkillTool
|
||||
* listing. Unconditional skills are always eligible; conditional skills
|
||||
* become eligible only after a tool invocation touches a file matching
|
||||
* their `paths:` globs.
|
||||
*/
|
||||
isSkillActive(skill: SkillConfig): boolean {
|
||||
if (!skill.paths || skill.paths.length === 0) return true;
|
||||
return this.activationRegistry?.isActivated(skill.name) ?? false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Activate any conditional skills whose `paths:` globs match `filePath`.
|
||||
* Returns the names of skills newly activated by this call. When at least
|
||||
* one skill activates, change listeners are notified so the SkillTool
|
||||
* description picks up the new entry on the next refresh.
|
||||
*/
|
||||
matchAndActivateByPath(filePath: string): string[] {
|
||||
if (!this.activationRegistry) return [];
|
||||
const newly = this.activationRegistry.matchAndConsume(filePath);
|
||||
if (newly.length > 0) {
|
||||
this.notifyChangeListeners();
|
||||
}
|
||||
return newly;
|
||||
}
|
||||
|
||||
/** Names of all conditional skills activated so far (read-only snapshot). */
|
||||
getActivatedSkillNames(): ReadonlySet<string> {
|
||||
return this.activationRegistry?.getActivatedNames() ?? new Set();
|
||||
}
|
||||
|
||||
/**
|
||||
* Starts watching skill directories for changes.
|
||||
*/
|
||||
|
|
@ -468,6 +516,20 @@ export class SkillManager {
|
|||
? true
|
||||
: undefined;
|
||||
|
||||
// Optional `paths` frontmatter: glob patterns that gate when this skill
|
||||
// is offered to the model (conditional skill).
|
||||
const pathsRaw = frontmatter['paths'];
|
||||
let paths: string[] | undefined;
|
||||
if (pathsRaw !== undefined) {
|
||||
if (!Array.isArray(pathsRaw)) {
|
||||
throw new Error('"paths" must be an array of glob patterns');
|
||||
}
|
||||
const cleaned = pathsRaw
|
||||
.map((p) => String(p).trim())
|
||||
.filter((p) => p.length > 0);
|
||||
paths = cleaned.length > 0 ? cleaned : undefined;
|
||||
}
|
||||
|
||||
const config: SkillConfig = {
|
||||
name,
|
||||
description,
|
||||
|
|
@ -480,6 +542,7 @@ export class SkillManager {
|
|||
body: body.trim(),
|
||||
whenToUse,
|
||||
disableModelInvocation,
|
||||
paths,
|
||||
};
|
||||
|
||||
// Validate the parsed configuration
|
||||
|
|
|
|||
|
|
@ -95,6 +95,16 @@ export interface SkillConfig {
|
|||
* field in SKILL.md.
|
||||
*/
|
||||
disableModelInvocation?: boolean;
|
||||
|
||||
/**
|
||||
* Optional glob patterns that gate when this skill is offered to the model.
|
||||
* When present and non-empty, the skill is a "conditional skill": it stays
|
||||
* out of the SkillTool listing until a tool invocation touches a file path
|
||||
* matching one of these patterns, at which point the skill is activated for
|
||||
* the rest of the session. Patterns are resolved relative to the project
|
||||
* root and matched via picomatch. Parsed from the `paths` frontmatter field.
|
||||
*/
|
||||
paths?: string[];
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -97,6 +97,10 @@ describe('SkillTool', () => {
|
|||
};
|
||||
}),
|
||||
getParseErrors: vi.fn().mockReturnValue(new Map()),
|
||||
// Default to "all skills active" so existing tests that use
|
||||
// unconditional skills are unaffected by the conditional-skill gating
|
||||
// added alongside `paths:` frontmatter.
|
||||
isSkillActive: vi.fn().mockReturnValue(true),
|
||||
} as unknown as SkillManager;
|
||||
|
||||
MockedSkillManager.mockImplementation(() => mockSkillManager);
|
||||
|
|
@ -243,6 +247,64 @@ describe('SkillTool', () => {
|
|||
'Skill "non-existent" not found. No skills are currently available.',
|
||||
);
|
||||
});
|
||||
|
||||
it('returns a path-activation error for a registered but not-yet-activated conditional skill', async () => {
|
||||
const conditionalSkill: SkillConfig = {
|
||||
name: 'tsx-helper',
|
||||
description: 'React TSX helper',
|
||||
level: 'project',
|
||||
filePath: '/test/project/.qwen/skills/tsx-helper/SKILL.md',
|
||||
body: 'Body.',
|
||||
paths: ['src/**/*.tsx'],
|
||||
};
|
||||
vi.mocked(mockSkillManager.listSkills).mockResolvedValue([
|
||||
conditionalSkill,
|
||||
]);
|
||||
// Simulate the skill being registered on disk but not yet activated.
|
||||
vi.mocked(mockSkillManager.isSkillActive).mockImplementation(
|
||||
(s: SkillConfig) => !s.paths || s.paths.length === 0,
|
||||
);
|
||||
|
||||
const gatedTool = new SkillTool(config);
|
||||
await vi.runAllTimersAsync();
|
||||
|
||||
const result = gatedTool.validateToolParams({ skill: 'tsx-helper' });
|
||||
expect(result).toMatch(/gated by path-based activation/);
|
||||
expect(result).toMatch(/paths: frontmatter/);
|
||||
});
|
||||
|
||||
it('does not allow a pending conditional skill to be invoked via the model-invocable command path', async () => {
|
||||
// Regression for /review finding: SkillCommandLoader exposes every
|
||||
// user/project skill as a model-invocable command. Without dropping
|
||||
// file-based names from modelInvocableCommands, validateToolParams
|
||||
// would accept a path-gated skill via the command branch and bypass
|
||||
// the activation contract entirely.
|
||||
const conditionalSkill: SkillConfig = {
|
||||
name: 'tsx-helper',
|
||||
description: 'React TSX helper',
|
||||
level: 'project',
|
||||
filePath: '/test/project/.qwen/skills/tsx-helper/SKILL.md',
|
||||
body: 'Body.',
|
||||
paths: ['src/**/*.tsx'],
|
||||
};
|
||||
vi.mocked(mockSkillManager.listSkills).mockResolvedValue([
|
||||
conditionalSkill,
|
||||
]);
|
||||
vi.mocked(mockSkillManager.isSkillActive).mockImplementation(
|
||||
(s: SkillConfig) => !s.paths || s.paths.length === 0,
|
||||
);
|
||||
// SkillCommandLoader would surface tsx-helper here even though it is
|
||||
// a path-gated file-based skill.
|
||||
vi.mocked(config.getModelInvocableCommandsProvider).mockReturnValue(
|
||||
() => [{ name: 'tsx-helper', description: 'React TSX helper' }],
|
||||
);
|
||||
|
||||
const gatedTool = new SkillTool(config);
|
||||
await vi.runAllTimersAsync();
|
||||
|
||||
const result = gatedTool.validateToolParams({ skill: 'tsx-helper' });
|
||||
expect(result).toMatch(/gated by path-based activation/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('refreshSkills', () => {
|
||||
|
|
|
|||
|
|
@ -35,6 +35,12 @@ export class SkillTool extends BaseDeclarativeTool<SkillParams, ToolResult> {
|
|||
|
||||
private skillManager: SkillManager;
|
||||
private availableSkills: SkillConfig[] = [];
|
||||
// Conditional skills (with `paths:`) that exist on disk but have not yet
|
||||
// been activated by a matching tool invocation. Tracked separately so
|
||||
// validateToolParams can give a distinct error message when the model
|
||||
// names one of these: "gated by paths:, access a matching file first"
|
||||
// instead of the generic "not found".
|
||||
private pendingConditionalSkillNames: Set<string> = new Set();
|
||||
private modelInvocableCommands: ReadonlyArray<{
|
||||
name: string;
|
||||
description: string;
|
||||
|
|
@ -85,22 +91,45 @@ export class SkillTool extends BaseDeclarativeTool<SkillParams, ToolResult> {
|
|||
*/
|
||||
async refreshSkills(): Promise<void> {
|
||||
try {
|
||||
this.availableSkills = (await this.skillManager.listSkills()).filter(
|
||||
(s) => !s.disableModelInvocation,
|
||||
// Include a skill in the tool description only when (a) it is not
|
||||
// hidden from the model (`disable-model-invocation`), and (b) it is
|
||||
// either unconditional or already activated by a matching file path
|
||||
// in this session. This keeps the tool description small in large
|
||||
// monorepos where most conditional skills are not yet relevant.
|
||||
const allSkills = await this.skillManager.listSkills();
|
||||
this.availableSkills = allSkills.filter(
|
||||
(s) => !s.disableModelInvocation && this.skillManager.isSkillActive(s),
|
||||
);
|
||||
// Merge in model-invocable commands from CommandService (injected via Config),
|
||||
// but exclude any whose names already appear as file-based skills to avoid
|
||||
// showing the same skill in both <available_skills> and <available_commands>.
|
||||
// Track still-pending conditional skills so validateToolParams can
|
||||
// distinguish "not found" from "registered but not yet activated".
|
||||
this.pendingConditionalSkillNames = new Set(
|
||||
allSkills
|
||||
.filter(
|
||||
(s) =>
|
||||
!s.disableModelInvocation &&
|
||||
s.paths &&
|
||||
s.paths.length > 0 &&
|
||||
!this.skillManager.isSkillActive(s),
|
||||
)
|
||||
.map((s) => s.name),
|
||||
);
|
||||
// Merge in model-invocable commands from CommandService (injected via
|
||||
// Config), but exclude any whose names appear as ANY file-based skill —
|
||||
// including pending conditional skills. Using `availableSkills` (active
|
||||
// only) here would let a path-gated skill leak through the
|
||||
// <available_commands> listing and bypass validateToolParams's
|
||||
// pendingConditionalSkillNames check, breaking the activation contract.
|
||||
const provider = this.config.getModelInvocableCommandsProvider();
|
||||
const allCommands = provider ? provider() : [];
|
||||
const skillNames = new Set(this.availableSkills.map((s) => s.name));
|
||||
const fileBasedSkillNames = new Set(allSkills.map((s) => s.name));
|
||||
this.modelInvocableCommands = allCommands.filter(
|
||||
(cmd) => !skillNames.has(cmd.name),
|
||||
(cmd) => !fileBasedSkillNames.has(cmd.name),
|
||||
);
|
||||
this.updateDescriptionAndSchema();
|
||||
} catch (error) {
|
||||
debugLogger.warn('Failed to load skills for Skills tool:', error);
|
||||
this.availableSkills = [];
|
||||
this.pendingConditionalSkillNames = new Set();
|
||||
this.modelInvocableCommands = [];
|
||||
this.updateDescriptionAndSchema();
|
||||
} finally {
|
||||
|
|
@ -208,6 +237,15 @@ ${skillDescriptions}
|
|||
);
|
||||
if (commandExists) return null;
|
||||
|
||||
// Distinct error for a conditional skill (registered via `paths:`
|
||||
// frontmatter) that has not yet been activated by a matching tool call.
|
||||
// Without this branch the model can't tell the difference between "no
|
||||
// such skill exists" and "exists but you need to access a matching file
|
||||
// to unlock it."
|
||||
if (this.pendingConditionalSkillNames.has(params.skill)) {
|
||||
return `Skill "${params.skill}" is gated by path-based activation (paths: frontmatter) and is not yet available. Access a file matching its paths patterns first to activate it.`;
|
||||
}
|
||||
|
||||
const availableNames = [
|
||||
...this.availableSkills.map((s) => s.name),
|
||||
...this.modelInvocableCommands.map((c) => c.name),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue