diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index a49dce42449..587560f0690 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -8501,6 +8501,11 @@ export class Config { * skills, user/project file commands, MCP prompts). Called by the CLI's * CommandService after initialisation so that the startup snapshot and * per-turn drain can include these in the `` listing. + * + * Unlike `disabledSkillNamesProvider`, late attachment (after + * `Config.initialize()` has warmed the tool registry) is supported: + * `SkillTool.validateToolParams` consults this provider live rather than + * relying on its construction-time snapshot (issue #9821). */ setModelInvocableCommandsProvider( provider: () => ReadonlyArray<{ name: string; description: string }>, diff --git a/packages/core/src/tools/skill-utils.ts b/packages/core/src/tools/skill-utils.ts index df3caee31d1..22bef9042ac 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -50,6 +50,8 @@ export interface CollectedAvailableSkills { pendingConditionalSkillNames: Set; /** Model-invocable commands, deduped against file-based skill names. */ modelInvocableCommands: ReadonlyArray<{ name: string; description: string }>; + /** File-based skills hidden from model invocation. */ + hiddenSkillNames?: Set; /** Normalized entries, ready for `renderAvailableSkillsBlock`. */ entries: AvailableSkillEntry[]; } @@ -144,6 +146,9 @@ async function collectAvailableSkillEntriesUncached( skillManager.isSkillActive(s) && !isDisabled(s.name), ); + const hiddenSkillNames = new Set( + allSkills.filter((s) => s.disableModelInvocation).map((s) => s.name), + ); // Track still-pending conditional skills so validation can emit a distinct // "gated by paths:" hint. Disabled conditional skills are excluded — no point @@ -196,6 +201,7 @@ async function collectAvailableSkillEntriesUncached( availableSkills, pendingConditionalSkillNames, modelInvocableCommands, + hiddenSkillNames, entries, }; } diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 420d9326bb5..b6c390c23f1 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -1270,6 +1270,34 @@ describe('SkillTool', () => { expect(result).toBeNull(); }); + it('should fall back to cached commands when the live provider throws', () => { + vi.mocked(config.getModelInvocableCommandsProvider).mockReturnValue( + () => { + throw new Error('boom'); + }, + ); + + const result = skillTool.validateToolParams({ skill: 'mcp-prompt-a' }); + expect(result).toBeNull(); + }); + + it('should accept a command with the same name as a hidden file skill', async () => { + vi.mocked(mockSkillManager.listSkills).mockResolvedValue([ + { + name: 'mcp-prompt-a', + description: 'Hidden file-based skill', + level: 'project', + filePath: '/test/project/.qwen/skills/mcp-prompt-a/SKILL.md', + body: 'Hidden body', + disableModelInvocation: true, + }, + ]); + await skillTool.refreshSkills(); + + const result = skillTool.validateToolParams({ skill: 'mcp-prompt-a' }); + expect(result).toBeNull(); + }); + it('should reject a name not in skills or commands, listing both in error', () => { const result = skillTool.validateToolParams({ skill: 'unknown' }); expect(result).toContain('"unknown" not found'); @@ -1278,6 +1306,83 @@ describe('SkillTool', () => { }); }); + // Regression for issue #9821. In interactive mode the + // modelInvocableCommands provider is only registered once the CLI's + // CommandService finishes initialising — AFTER `Config.initialize()` → + // `toolRegistry.warmAll()` has already constructed SkillTool, whose + // constructor `refreshSkills()` therefore read a still-null provider and + // cached an empty command set. Nothing re-notifies SkillTool when the + // provider is attached, so validation kept rejecting every command until + // an unrelated SkillManager change event happened to re-run + // `refreshSkills()` — the source of the reported ~50% flakiness. + describe('late-attached modelInvocableCommands provider (issue #9821)', () => { + it('validates a command registered after construction with no SkillManager change event', () => { + // beforeEach already constructed skillTool with a null provider and + // drained the constructor's refreshSkills(). Register the provider + // late — deliberately WITHOUT firing a change listener — mirroring + // slashCommandProcessor's post-CommandService.create registration. + vi.mocked(config.getModelInvocableCommandsProvider).mockReturnValue( + () => [ + { name: 'late-command', description: 'Registered after startup' }, + ], + ); + + expect( + skillTool.validateToolParams({ skill: 'late-command' }), + ).toBeNull(); + }); + + it('lists late-registered commands in the not-found error', () => { + vi.mocked(config.getModelInvocableCommandsProvider).mockReturnValue( + () => [ + { name: 'late-command', description: 'Registered after startup' }, + ], + ); + + const result = skillTool.validateToolParams({ skill: 'unknown' }); + expect(result).toContain('"unknown" not found'); + expect(result).toContain('late-command'); + }); + + it('keeps path-gated skills gated when the provider is late-registered', async () => { + // The live read must apply the same file-based-skill name shadowing + // as collectAvailableSkillEntries, so a command named after a pending + // conditional skill cannot bypass the "gated by paths:" branch. + 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, + ); + const gatedTool = new SkillTool(config); + await vi.runAllTimersAsync(); + + // Late provider registration (SkillCommandLoader surfaces the skill). + vi.mocked(config.getModelInvocableCommandsProvider).mockReturnValue( + () => [{ name: 'tsx-helper', description: 'React TSX helper' }], + ); + + const result = gatedTool.validateToolParams({ skill: 'tsx-helper' }); + expect(result).toMatch(/gated by path-based activation/); + }); + + it('still rejects unknown commands when no provider is ever registered', () => { + // SDK/headless mode without a provider: behavior must be unchanged. + const result = skillTool.validateToolParams({ skill: 'unknown' }); + expect(result).toBe( + 'Skill "unknown" not found. Available skills: code-review, testing', + ); + }); + }); + describe('commandExecutor fallback in execute()', () => { beforeEach(async () => { // Expose an MCP-only command that has no file-based skill @@ -1417,6 +1522,151 @@ describe('SkillTool', () => { }); describe('disabled-skill execute guard', () => { + const createHiddenSkillInvocation = async ( + executor: ReturnType, + params: SkillParams = { skill: 'mcp-prompt-a' }, + ) => { + vi.mocked(mockSkillManager.listSkills).mockResolvedValue([ + { + name: 'mcp-prompt-a', + description: 'Hidden file-based skill', + level: 'project', + filePath: '/test/project/.qwen/skills/mcp-prompt-a/SKILL.md', + body: 'HIDDEN skill body must not execute', + disableModelInvocation: true, + }, + ]); + const hiddenAwareTool = new SkillTool(config); + await vi.runAllTimersAsync(); + vi.mocked(config.getModelInvocableCommandsExecutor).mockReturnValue( + executor, + ); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + name: 'mcp-prompt-a', + description: 'Hidden file-based skill', + level: 'project', + filePath: '/test/project/.qwen/skills/mcp-prompt-a/SKILL.md', + body: 'HIDDEN skill body must not execute', + disableModelInvocation: true, + }); + + return ( + hiddenAwareTool as SkillToolWithProtectedMethods + ).createInvocation(params); + }; + + it('runs the same-named MCP prompt instead of loading a hidden skill', async () => { + vi.mocked(mockSkillManager.listSkills).mockResolvedValue([ + { + name: 'mcp-prompt-a', + description: 'Hidden file-based skill', + level: 'project', + filePath: '/test/project/.qwen/skills/mcp-prompt-a/SKILL.md', + body: 'HIDDEN skill body must not execute', + disableModelInvocation: true, + }, + ]); + const hiddenAwareTool = new SkillTool(config); + await vi.runAllTimersAsync(); + const executor = vi.fn().mockResolvedValue('MCP prompt body'); + vi.mocked(config.getModelInvocableCommandsExecutor).mockReturnValue( + executor, + ); + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + name: 'mcp-prompt-a', + description: 'Hidden file-based skill', + level: 'project', + filePath: '/test/project/.qwen/skills/mcp-prompt-a/SKILL.md', + body: 'HIDDEN skill body must not execute', + disableModelInvocation: true, + }); + + const invocation = ( + hiddenAwareTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'mcp-prompt-a' }); + const result = await invocation.execute(); + + expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled(); + expect(executor).toHaveBeenCalledWith('mcp-prompt-a', ''); + expect(partToString(result.llmContent)).toBe('MCP prompt body'); + expect(result.returnDisplay).toBe('Delegated to command: mcp-prompt-a'); + }); + + it('returns command executor errors for hidden skill command alternatives', async () => { + const executor = vi + .fn() + .mockResolvedValue({ error: 'MCP prompt failed' }); + const invocation = await createHiddenSkillInvocation(executor); + const result = await invocation.execute(); + + expect(executor).toHaveBeenCalledWith('mcp-prompt-a', ''); + expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled(); + expect(partToString(result.llmContent)).toBe('MCP prompt failed'); + expect(result.returnDisplay).toBe('MCP prompt failed'); + expect(recordSkillInvocation).not.toHaveBeenCalled(); + }); + + it('passes args to command alternatives for hidden skills', async () => { + const executor = vi.fn().mockResolvedValue('MCP prompt body'); + const invocation = await createHiddenSkillInvocation(executor, { + skill: 'mcp-prompt-a', + args: 'arg text', + }); + await invocation.execute(); + + expect(executor).toHaveBeenCalledWith('mcp-prompt-a', 'arg text'); + expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled(); + }); + + it('falls through to not-found when hidden skill commandExecutor throws', async () => { + const executor = vi.fn().mockRejectedValue(new Error('MCP timeout')); + const invocation = await createHiddenSkillInvocation(executor); + const result = await invocation.execute(); + + expect(executor).toHaveBeenCalledWith('mcp-prompt-a', ''); + expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled(); + expect(partToString(result.llmContent)).toBe( + 'Skill "mcp-prompt-a" not found.', + ); + expect(recordSkillInvocation).not.toHaveBeenCalled(); + }); + + it('returns not-found when a hidden skill command alternative returns null', async () => { + const executor = vi.fn().mockResolvedValue(null); + const invocation = await createHiddenSkillInvocation(executor); + const result = await invocation.execute(); + + expect(executor).toHaveBeenCalledWith('mcp-prompt-a', ''); + expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled(); + expect(partToString(result.llmContent)).toBe( + 'Skill "mcp-prompt-a" not found.', + ); + expect(recordSkillInvocation).not.toHaveBeenCalled(); + }); + + it('returns not-found and records failure when no hidden skill command alternative exists', async () => { + const invocation = await createHiddenSkillInvocation(null); + invocation.setPromptId('prompt-123'); + const result = await invocation.execute(); + + expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled(); + expect(partToString(result.llmContent)).toBe( + 'Skill "mcp-prompt-a" not found.', + ); + expect(logSkillLaunch).toHaveBeenCalledWith( + config, + expect.objectContaining({ + skill_name: 'mcp-prompt-a', + success: false, + prompt_id: 'prompt-123', + }), + ); + expect(recordSkillInvocation).toHaveBeenCalledWith(config, { + skillName: 'mcp-prompt-a', + success: false, + }); + }); + it('runs the same-named MCP prompt instead of loading a disabled skill', async () => { // Regression: without the execute-side guard, // `loadSkillForRuntime` resolves the disabled skill from disk and diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index 58d426a7828..6804b69b3c7 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -97,6 +97,7 @@ export class SkillTool extends BaseDeclarativeTool { name: string; description: string; }> = []; + private hiddenSkillNames: Set = new Set(); private loadedSkillNames: Set = new Set(); // Cleanup function returned by `addChangeListener`. Stored so per-agent // SkillTool instances (subagents share the parent's SkillManager) can @@ -184,11 +185,13 @@ export class SkillTool extends BaseDeclarativeTool { this.pendingConditionalSkillNames = collected.pendingConditionalSkillNames; this.modelInvocableCommands = collected.modelInvocableCommands; + this.hiddenSkillNames = collected.hiddenSkillNames ?? new Set(); } catch (error) { debugLogger.warn('Failed to load skills for Skills tool:', error); this.availableSkills = []; this.pendingConditionalSkillNames = new Set(); this.modelInvocableCommands = []; + this.hiddenSkillNames = new Set(); } } @@ -211,8 +214,14 @@ export class SkillTool extends BaseDeclarativeTool { ); if (skillExists) return null; - // Check model-invocable commands (e.g. MCP prompts) listed in - const commandExists = this.modelInvocableCommands.some( + // Check model-invocable commands (e.g. MCP prompts) listed in + // . Consults the live provider — not just the cached + // snapshot — because in interactive mode the provider is only attached + // after CommandService initialisation resolves, which races SkillTool + // construction: the constructor's refreshSkills() then reads a still-null + // provider and caches an empty command set that is never refreshed unless + // an unrelated SkillManager change event happens to fire (issue #9821). + const commandExists = this.getModelInvocableCommands().some( (cmd) => cmd.name === params.skill, ); if (commandExists) return null; @@ -236,8 +245,10 @@ export class SkillTool extends BaseDeclarativeTool { } const availableNames = [ - ...this.availableSkills.map((s) => s.name), - ...this.modelInvocableCommands.map((c) => c.name), + ...new Set([ + ...this.availableSkills.map((s) => s.name), + ...this.getModelInvocableCommands().map((c) => c.name), + ]), ]; if (availableNames.length === 0) { return `Skill "${params.skill}" not found. No skills are currently available.`; @@ -245,6 +256,46 @@ export class SkillTool extends BaseDeclarativeTool { return `Skill "${params.skill}" not found. Available skills: ${availableNames.join(', ')}`; } + /** + * Returns the model-invocable commands to validate against, preferring a + * live read of the config provider over the cached snapshot from the last + * `refreshSkills()` (see `validateToolParams` for the late-attach race). + * Falls back to the cache when no provider is registered (e.g. SDK mode) + * or when the provider throws. The provider is synchronous, so the live + * read is cheap enough to run on every validation. + * + * Commands whose names collide with a file-based skill (active or pending + * path-activation) are dropped, mirroring the `fileBasedSkillNames` dedup + * in `collectAvailableSkillEntries` — without this, a command named after + * a path-gated skill would pass validation here and bypass the + * "gated by paths:" branch above. + */ + private getModelInvocableCommands(): ReadonlyArray<{ + name: string; + description: string; + }> { + let commands: ReadonlyArray<{ name: string; description: string }>; + const provider = this.config.getModelInvocableCommandsProvider(); + if (provider) { + try { + commands = provider(); + } catch (error) { + debugLogger.warn( + 'Model-invocable commands provider threw; falling back to cached set:', + error, + ); + commands = this.modelInvocableCommands; + } + } else { + commands = this.modelInvocableCommands; + } + const shadowedNames = new Set([ + ...this.availableSkills.map((skill) => skill.name), + ...this.pendingConditionalSkillNames, + ]); + return commands.filter((cmd) => !shadowedNames.has(cmd.name)); + } + protected createInvocation(params: SkillParams) { return new SkillToolInvocation( this.config, @@ -253,6 +304,7 @@ export class SkillTool extends BaseDeclarativeTool { (name: string) => this.loadedSkillNames.add(name), this.config.getModelInvocableCommandsExecutor(), (name: string) => this.loadedSkillNames.has(name), + (name: string) => this.hiddenSkillNames.has(name), ); } @@ -315,6 +367,7 @@ class SkillToolInvocation extends BaseToolInvocation { ) => Promise) | null = null, private readonly isSkillLoaded: (name: string) => boolean = () => false, + private readonly isSkillHidden: (name: string) => boolean = () => false, ) { super(params); } @@ -357,6 +410,49 @@ class SkillToolInvocation extends BaseToolInvocation { _signal?: AbortSignal, _updateOutput?: (output: ToolResultDisplay) => void, ): Promise { + if (this.isSkillHidden(this.params.skill)) { + let hiddenCommandFallbackAttempted = false; + if (this.commandExecutor) { + hiddenCommandFallbackAttempted = true; + try { + const content = await this.commandExecutor( + this.params.skill, + this.params.args ?? '', + ); + if (content && typeof content === 'object' && 'error' in content) { + return { + llmContent: content.error, + returnDisplay: content.error, + }; + } + if (typeof content === 'string') { + return { + llmContent: [{ text: content }], + returnDisplay: `Delegated to command: ${this.params.skill}`, + }; + } + } catch (error) { + debugLogger.warn( + `Hidden-skill command fallback failed for "${this.params.skill}":`, + error, + ); + // Fall through to the generic not-found message. + } + } + logSkillLaunch( + this.config, + new SkillLaunchEvent(this.params.skill, false, this.promptId), + ); + if (!hiddenCommandFallbackAttempted) { + recordSkillInvocation(this.config, { + skillName: this.params.skill, + success: false, + }); + } + const msg = `Skill "${this.params.skill}" not found.`; + return { llmContent: msg, returnDisplay: msg }; + } + // Disabled-skill guard. Mirrors validateToolParams's commandExists → // disabled ordering at the execution layer: when a skill is disabled // but a same-named non-skill command (MCP prompt, file command)