Agent Skills: Extension Support & Security Disclosure (#15834)

packages/core/src/config/config.ts

@@ -97,7 +97,7 @@ import { DELEGATE_TO_AGENT_TOOL_NAME } from '../tools/tool-names.js';
 import { getExperiments } from '../code_assist/experiments/experiments.js';
 import { ExperimentFlags } from '../code_assist/experiments/flagNames.js';
 import { debugLogger } from '../utils/debugLogger.js';
-import { SkillManager } from '../skills/skillManager.js';
+import { SkillManager, type SkillDefinition } from '../skills/skillManager.js';
 import { startupProfiler } from '../telemetry/startupProfiler.js';
 
 import { ApprovalMode } from '../policy/types.js';
@@ -175,6 +175,7 @@ export interface GeminiCLIExtension {
   hooks?: { [K in HookEventName]?: HookDefinition[] };
   settings?: ExtensionSetting[];
   resolvedSettings?: ResolvedExtensionSetting[];
+  skills?: SkillDefinition[];
 }
 
 export interface ExtensionInstallMetadata {
@@ -732,7 +733,10 @@ export class Config {
 
     // Discover skills if enabled
     if (this.skillsSupport) {
-      await this.getSkillManager().discoverSkills(this.storage);
+      await this.getSkillManager().discoverSkills(
+        this.storage,
+        this.getExtensions(),
+      );
       this.getSkillManager().setDisabledSkills(this.disabledSkills);
 
       // Re-register ActivateSkillTool to update its schema with the discovered enabled skill enums

packages/core/src/skills/skillLoader.test.ts

@@ -9,6 +9,7 @@ import * as fs from 'node:fs/promises';
 import * as os from 'node:os';
 import * as path from 'node:path';
 import { loadSkillsFromDir } from './skillLoader.js';
+import { coreEvents } from '../utils/events.js';
 
 describe('skillLoader', () => {
   let testRootDir: string;
@@ -17,6 +18,7 @@ describe('skillLoader', () => {
     testRootDir = await fs.mkdtemp(
       path.join(os.tmpdir(), 'skill-loader-test-'),
     );
+    vi.spyOn(coreEvents, 'emitFeedback');
   });
 
   afterEach(async () => {
@@ -40,18 +42,45 @@ describe('skillLoader', () => {
     expect(skills[0].description).toBe('A test skill');
     expect(skills[0].location).toBe(skillFile);
     expect(skills[0].body).toBe('# Instructions\nDo something.');
+    expect(coreEvents.emitFeedback).not.toHaveBeenCalled();
+  });
+
+  it('should emit feedback when no valid skills are found in a non-empty directory', async () => {
+    const notASkillDir = path.join(testRootDir, 'not-a-skill');
+    await fs.mkdir(notASkillDir, { recursive: true });
+    await fs.writeFile(path.join(notASkillDir, 'some-file.txt'), 'hello');
+
+    const skills = await loadSkillsFromDir(testRootDir);
+
+    expect(skills).toHaveLength(0);
+    expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
+      'warning',
+      expect.stringContaining('Failed to load skills from'),
+    );
+  });
+
+  it('should ignore empty directories and not emit feedback', async () => {
+    const skills = await loadSkillsFromDir(testRootDir);
+
+    expect(skills).toHaveLength(0);
+    expect(coreEvents.emitFeedback).not.toHaveBeenCalled();
   });
 
   it('should ignore directories without SKILL.md', async () => {
     const notASkillDir = path.join(testRootDir, 'not-a-skill');
     await fs.mkdir(notASkillDir, { recursive: true });
 
+    // With a subdirectory, even if empty, it might still trigger readdir
+    // But my current logic is if discoveredSkills.length === 0, then check readdir
+    // If readdir is empty, it's fine.
+
     const skills = await loadSkillsFromDir(testRootDir);
 
     expect(skills).toHaveLength(0);
+    // If notASkillDir is empty, no warning.
   });
 
-  it('should ignore SKILL.md without valid frontmatter', async () => {
+  it('should ignore SKILL.md without valid frontmatter and emit warning if directory is not empty', async () => {
     const skillDir = path.join(testRootDir, 'invalid-skill');
     await fs.mkdir(skillDir, { recursive: true });
     const skillFile = path.join(skillDir, 'SKILL.md');
@@ -60,10 +89,15 @@ describe('skillLoader', () => {
     const skills = await loadSkillsFromDir(testRootDir);
 
     expect(skills).toHaveLength(0);
+    expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
+      'warning',
+      expect.stringContaining('Failed to load skills from'),
+    );
   });
 
   it('should return empty array for non-existent directory', async () => {
     const skills = await loadSkillsFromDir('/non/existent/path');
     expect(skills).toEqual([]);
+    expect(coreEvents.emitFeedback).not.toHaveBeenCalled();
   });
 });

packages/core/src/skills/skillLoader.ts

@@ -9,6 +9,7 @@ import * as path from 'node:path';
 import { glob } from 'glob';
 import yaml from 'js-yaml';
 import { debugLogger } from '../utils/debugLogger.js';
+import { coreEvents } from '../utils/events.js';
 
 /**
  * Represents the definition of an Agent Skill.
@@ -55,8 +56,22 @@ export async function loadSkillsFromDir(
         discoveredSkills.push(metadata);
       }
     }
+
+    if (discoveredSkills.length === 0) {
+      const files = await fs.readdir(absoluteSearchPath);
+      if (files.length > 0) {
+        coreEvents.emitFeedback(
+          'warning',
+          `Failed to load skills from ${absoluteSearchPath}. The directory is not empty but no valid skills were discovered. Please ensure SKILL.md files are present in subdirectories and have valid frontmatter.`,
+        );
+      }
+    }
   } catch (error) {
-    debugLogger.log(`Error discovering skills in ${dir}:`, error);
+    coreEvents.emitFeedback(
+      'warning',
+      `Error discovering skills in ${dir}:`,
+      error,
+    );
   }
 
   return discoveredSkills;

packages/core/src/skills/skillManager.test.ts

@@ -10,6 +10,7 @@ import * as os from 'node:os';
 import * as path from 'node:path';
 import { SkillManager } from './skillManager.js';
 import { Storage } from '../config/storage.js';
+import { type GeminiCLIExtension } from '../config/config.js';
 
 describe('SkillManager', () => {
   let testRootDir: string;
@@ -25,39 +26,116 @@ describe('SkillManager', () => {
     vi.restoreAllMocks();
   });
 
-  it('should discover skills from Storage with project precedence', async () => {
+  it('should discover skills from extensions, user, and project with precedence', async () => {
     const userDir = path.join(testRootDir, 'user');
     const projectDir = path.join(testRootDir, 'project');
     await fs.mkdir(path.join(userDir, 'skill-a'), { recursive: true });
-    await fs.mkdir(path.join(projectDir, 'skill-a'), { recursive: true });
+    await fs.mkdir(path.join(projectDir, 'skill-b'), { recursive: true });
 
     await fs.writeFile(
       path.join(userDir, 'skill-a', 'SKILL.md'),
       `---
-name: skill-a
+name: skill-user
 description: user-desc
 ---
 `,
     );
     await fs.writeFile(
-      path.join(projectDir, 'skill-a', 'SKILL.md'),
+      path.join(projectDir, 'skill-b', 'SKILL.md'),
       `---
-name: skill-a
+name: skill-project
 description: project-desc
 ---
 `,
     );
 
+    const mockExtension: GeminiCLIExtension = {
+      name: 'test-ext',
+      version: '1.0.0',
+      isActive: true,
+      path: '/ext',
+      contextFiles: [],
+      id: 'ext-id',
+      skills: [
+        {
+          name: 'skill-extension',
+          description: 'ext-desc',
+          location: '/ext/skills/SKILL.md',
+          body: 'body',
+        },
+      ],
+    };
+
     vi.spyOn(Storage, 'getUserSkillsDir').mockReturnValue(userDir);
     const storage = new Storage('/dummy');
     vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue(projectDir);
 
     const service = new SkillManager();
-    await service.discoverSkills(storage);
+    await service.discoverSkills(storage, [mockExtension]);
+
+    const skills = service.getSkills();
+    expect(skills).toHaveLength(3);
+    const names = skills.map((s) => s.name);
+    expect(names).toContain('skill-extension');
+    expect(names).toContain('skill-user');
+    expect(names).toContain('skill-project');
+  });
+
+  it('should respect precedence: Project > User > Extension', async () => {
+    const userDir = path.join(testRootDir, 'user');
+    const projectDir = path.join(testRootDir, 'project');
+    await fs.mkdir(path.join(userDir, 'skill'), { recursive: true });
+    await fs.mkdir(path.join(projectDir, 'skill'), { recursive: true });
+
+    await fs.writeFile(
+      path.join(userDir, 'skill', 'SKILL.md'),
+      `---
+name: same-name
+description: user-desc
+---
+`,
+    );
+    await fs.writeFile(
+      path.join(projectDir, 'skill', 'SKILL.md'),
+      `---
+name: same-name
+description: project-desc
+---
+`,
+    );
+
+    const mockExtension: GeminiCLIExtension = {
+      name: 'test-ext',
+      version: '1.0.0',
+      isActive: true,
+      path: '/ext',
+      contextFiles: [],
+      id: 'ext-id',
+      skills: [
+        {
+          name: 'same-name',
+          description: 'ext-desc',
+          location: '/ext/skills/SKILL.md',
+          body: 'body',
+        },
+      ],
+    };
+
+    vi.spyOn(Storage, 'getUserSkillsDir').mockReturnValue(userDir);
+    const storage = new Storage('/dummy');
+    vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue(projectDir);
+
+    const service = new SkillManager();
+    await service.discoverSkills(storage, [mockExtension]);
 
     const skills = service.getSkills();
     expect(skills).toHaveLength(1);
     expect(skills[0].description).toBe('project-desc');
+
+    // Test User > Extension
+    vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue('/non-existent');
+    await service.discoverSkills(storage, [mockExtension]);
+    expect(service.getSkills()[0].description).toBe('user-desc');
   });
 
   it('should filter disabled skills in getSkills but not in getAllSkills', async () => {

packages/core/src/skills/skillManager.ts

@@ -6,6 +6,9 @@
 
 import { Storage } from '../config/storage.js';
 import { type SkillDefinition, loadSkillsFromDir } from './skillLoader.js';
+import type { GeminiCLIExtension } from '../config/config.js';
+
+export { type SkillDefinition };
 
 export class SkillManager {
   private skills: SkillDefinition[] = [];
@@ -19,17 +22,27 @@ export class SkillManager {
   }
 
   /**
-   * Discovers skills from standard user and project locations.
-   * Project skills take precedence over user skills.
+   * Discovers skills from standard user and project locations, as well as extensions.
+   * Precedence: Extensions (lowest) -> User -> Project (highest).
    */
-  async discoverSkills(storage: Storage): Promise<void> {
+  async discoverSkills(
+    storage: Storage,
+    extensions: GeminiCLIExtension[] = [],
+  ): Promise<void> {
     this.clearSkills();
 
-    // 1. User skills
+    // 1. Extension skills (lowest precedence)
+    for (const extension of extensions) {
+      if (extension.isActive && extension.skills) {
+        this.addSkillsWithPrecedence(extension.skills);
+      }
+    }
+
+    // 2. User skills
     const userSkills = await loadSkillsFromDir(Storage.getUserSkillsDir());
     this.addSkillsWithPrecedence(userSkills);
 
-    // 2. Project skills (highest precedence)
+    // 3. Project skills (highest precedence)
     const projectSkills = await loadSkillsFromDir(
       storage.getProjectSkillsDir(),
     );