Merge branch 'main' into feature/support-agents-directory-skills

packages/core/src/skills/bundled/review/SKILL.md

@@ -0,0 +1,118 @@
+---
+name: review
+description: Review changed code for correctness, security, code quality, and performance. Use when the user asks to review code changes, a PR, or specific files. Invoke with `/review`, `/review <pr-number>`, or `/review <file-path>`.
+allowedTools:
+  - task
+  - run_shell_command
+  - grep_search
+  - read_file
+  - glob
+---
+
+# Code Review
+
+You are an expert code reviewer. Your job is to review code changes and provide actionable feedback.
+
+## Step 1: Determine what to review
+
+Based on the arguments provided:
+
+- **No arguments**: Review local uncommitted changes
+  - Run `git diff` and `git diff --staged` to get all changes
+  - If both diffs are empty, inform the user there are no changes to review and stop here — do not proceed to the review agents
+
+- **PR number or URL** (e.g., `123` or `https://github.com/.../pull/123`):
+  - Run `gh pr view <number>` to get PR details
+  - Run `gh pr diff <number>` to get the diff
+
+- **File path** (e.g., `src/foo.ts`):
+  - Run `git diff HEAD -- <file>` to get recent changes
+  - If no diff, read the file and review its current state
+
+## Step 2: Parallel multi-dimensional review
+
+Launch **four parallel review agents** to analyze the changes from different angles. Each agent should focus exclusively on its dimension.
+
+### Agent 1: Correctness & Security
+
+Focus areas:
+
+- Logic errors and edge cases
+- Null/undefined handling
+- Race conditions and concurrency issues
+- Security vulnerabilities (injection, XSS, SSRF, path traversal, etc.)
+- Type safety issues
+- Error handling gaps
+
+### Agent 2: Code Quality
+
+Focus areas:
+
+- Code style consistency with the surrounding codebase
+- Naming conventions (variables, functions, classes)
+- Code duplication and opportunities for reuse
+- Over-engineering or unnecessary abstraction
+- Missing or misleading comments
+- Dead code
+
+### Agent 3: Performance & Efficiency
+
+Focus areas:
+
+- Performance bottlenecks (N+1 queries, unnecessary loops, etc.)
+- Memory leaks or excessive memory usage
+- Unnecessary re-renders (for UI code)
+- Inefficient algorithms or data structures
+- Missing caching opportunities
+- Bundle size impact
+
+### Agent 4: Undirected Audit
+
+No preset dimension. Review the code with a completely fresh perspective to catch issues the other three agents may miss.
+Focus areas:
+
+- Business logic soundness and correctness of assumptions
+- Boundary interactions between modules or services
+- Implicit assumptions that may break under different conditions
+- Unexpected side effects or hidden coupling
+- Anything else that looks off — trust your instincts
+
+## Step 3: Aggregate and present findings
+
+Combine results from all four agents into a single, well-organized review. Use this format:
+
+### Summary
+
+A 1-2 sentence overview of the changes and overall assessment.
+
+### Findings
+
+Use severity levels:
+
+- **Critical** — Must fix before merging. Bugs, security issues, data loss risks.
+- **Suggestion** — Recommended improvement. Better patterns, clearer code, potential issues.
+- **Nice to have** — Optional optimization. Minor style tweaks, small performance gains.
+
+For each finding, include:
+
+1. **File and line reference** (e.g., `src/foo.ts:42`)
+2. **What's wrong** — Clear description of the issue
+3. **Why it matters** — Impact if not addressed
+4. **Suggested fix** — Concrete code suggestion when possible
+
+### Verdict
+
+One of:
+
+- **Approve** — No critical issues, good to merge
+- **Request changes** — Has critical issues that need fixing
+- **Comment** — Has suggestions but no blockers
+
+## Guidelines
+
+- Be specific and actionable. Avoid vague feedback like "could be improved."
+- Reference the existing codebase conventions — don't impose external style preferences.
+- Focus on the diff, not pre-existing issues in unchanged code.
+- Keep the review concise. Don't repeat the same point for every occurrence.
+- When suggesting a fix, show the actual code change.
+- Flag any exposed secrets, credentials, API keys, or tokens in the diff as **Critical**.

packages/core/src/skills/index.ts

@@ -11,9 +11,13 @@
  * users to define reusable skill configurations that can be loaded by the
  * model via a dedicated Skills tool.
  *
- * Skills are stored as directories in `.qwen/skills/` (project-level) or
- * `~/.qwen/skills/` (user-level), with each directory containing a SKILL.md
- * file with YAML frontmatter for metadata.
+ * Skills are stored as directories containing a SKILL.md file with YAML
+ * frontmatter for metadata. They can be loaded from four levels
+ * (precedence: project > user > extension > bundled):
+ * - Project-level: `.qwen/skills/`
+ * - User-level: `~/.qwen/skills/`
+ * - Extension-level: provided by installed extensions
+ * - Bundled: built-in skills shipped with qwen-code
  */
 
 // Core types and interfaces

packages/core/src/skills/skill-manager.test.ts

@@ -621,6 +621,118 @@ Skill 3 content`);
       expect(baseDirs).toContain(path.join('/home/user', '.codex', 'skills'));
       expect(baseDirs).toContain(path.join('/home/user', '.claude', 'skills'));
     });
+
+    it('should return bundled-level base dir', () => {
+      const baseDir = manager.getSkillsBaseDir('bundled');
+
+      expect(baseDir).toMatch(/skills[/\\]bundled$/);
+    });
+
+    it('should throw for extension level', () => {
+      expect(() => manager.getSkillsBaseDir('extension')).toThrow(
+        'Extension skills do not have a base directory',
+      );
+    });
+  });
+
+  describe('bundled skills', () => {
+    const bundledDirSegment = path.join('skills', 'bundled');
+    const projectDirSegment = path.join('.qwen', 'skills');
+    const userDirSegment = path.join('.qwen', 'skills');
+    const projectPrefix = path.join('/test/project');
+    const userPrefix = path.join('/home/user');
+
+    const reviewDirEntry = {
+      name: 'review',
+      isDirectory: () => true,
+      isFile: () => false,
+      isSymbolicLink: () => false,
+    };
+
+    const emptyDir = [] as unknown as Awaited<ReturnType<typeof fs.readdir>>;
+
+    function mockReaddirForLevels(levels: Set<string>) {
+      vi.mocked(fs.readdir).mockImplementation((dirPath) => {
+        const pathStr = String(dirPath);
+        const isBundled =
+          pathStr.endsWith(bundledDirSegment) && !pathStr.includes('.qwen');
+        const isProject =
+          pathStr.includes(projectDirSegment) &&
+          pathStr.startsWith(projectPrefix);
+        const isUser =
+          pathStr.includes(userDirSegment) && pathStr.startsWith(userPrefix);
+
+        if (
+          (levels.has('bundled') && isBundled) ||
+          (levels.has('project') && isProject) ||
+          (levels.has('user') && isUser)
+        ) {
+          return Promise.resolve([reviewDirEntry] as unknown as Awaited<
+            ReturnType<typeof fs.readdir>
+          >);
+        }
+        return Promise.resolve(emptyDir);
+      });
+    }
+
+    function setupReviewSkillMocks() {
+      vi.mocked(fs.access).mockResolvedValue(undefined);
+      vi.mocked(fs.readFile).mockResolvedValue(`---
+name: review
+description: Review code changes
+---
+Review content`);
+
+      mockParseYaml.mockReturnValue({
+        name: 'review',
+        description: 'Review code changes',
+      });
+    }
+
+    it('should load bundled skills in listSkills', async () => {
+      mockReaddirForLevels(new Set(['bundled']));
+      setupReviewSkillMocks();
+
+      const skills = await manager.listSkills({ force: true });
+
+      expect(skills.some((s) => s.name === 'review')).toBe(true);
+      const reviewSkill = skills.find((s) => s.name === 'review');
+      expect(reviewSkill!.level).toBe('bundled');
+    });
+
+    it('should prioritize project-level over bundled skills with same name', async () => {
+      mockReaddirForLevels(new Set(['project', 'bundled']));
+      setupReviewSkillMocks();
+
+      const skills = await manager.listSkills({ force: true });
+
+      const reviewSkills = skills.filter((s) => s.name === 'review');
+      expect(reviewSkills).toHaveLength(1);
+      expect(reviewSkills[0].level).toBe('project');
+    });
+
+    it('should prioritize user-level over bundled skills with same name', async () => {
+      mockReaddirForLevels(new Set(['user', 'bundled']));
+      setupReviewSkillMocks();
+
+      const skills = await manager.listSkills({ force: true });
+
+      const reviewSkills = skills.filter((s) => s.name === 'review');
+      expect(reviewSkills).toHaveLength(1);
+      expect(reviewSkills[0].level).toBe('user');
+    });
+
+    it('should fall back to bundled level in loadSkill', async () => {
+      // Project, user, extension all empty; bundled has the skill
+      mockReaddirForLevels(new Set(['bundled']));
+      setupReviewSkillMocks();
+
+      const skill = await manager.loadSkill('review');
+
+      expect(skill).toBeDefined();
+      expect(skill!.name).toBe('review');
+      expect(skill!.level).toBe('bundled');
+    });
   });
 
   describe('change listeners', () => {

packages/core/src/skills/skill-manager.ts

@@ -8,6 +8,7 @@ import * as fs from 'fs/promises';
 import * as fsSync from 'fs';
 import * as path from 'path';
 import * as os from 'os';
+import { fileURLToPath } from 'url';
 import { watch as watchFs, type FSWatcher } from 'chokidar';
 import { parse as parseYaml } from '../utils/yaml-parser.js';
 import type {
@@ -40,8 +41,14 @@ export class SkillManager {
   private readonly watchers: Map<string, FSWatcher> = new Map();
   private watchStarted = false;
   private refreshTimer: NodeJS.Timeout | null = null;
+  private readonly bundledSkillsDir: string;
 
-  constructor(private readonly config: Config) {}
+  constructor(private readonly config: Config) {
+    this.bundledSkillsDir = path.join(
+      path.dirname(fileURLToPath(import.meta.url)),
+      'bundled',
+    );
+  }
 
   /**
    * Adds a listener that will be called when skills change.
@@ -90,7 +97,7 @@ export class SkillManager {
 
     const levelsToCheck: SkillLevel[] = options.level
       ? [options.level]
-      : ['project', 'user', 'extension'];
+      : ['project', 'user', 'extension', 'bundled'];
 
     // Check if we should use cache or force refresh
     const shouldUseCache = !options.force && this.skillsCache !== null;
@@ -103,7 +110,7 @@ export class SkillManager {
       debugLogger.debug('Using cached skills');
     }
 
-    // Collect skills from each level (project takes precedence over user over extension)
+    // Collect skills from each level (precedence: project > user > extension > bundled)
     for (const level of levelsToCheck) {
       const levelSkills = this.skillsCache?.get(level) || [];
       debugLogger.debug(
@@ -111,7 +118,7 @@ export class SkillManager {
       );
 
       for (const skill of levelSkills) {
-        // Skip if we've already seen this name (precedence: project > user > extension)
+        // Skip if we've already seen this name (precedence: project > user > extension > bundled)
         if (seenNames.has(skill.name)) {
           debugLogger.debug(
             `Skipping duplicate skill: ${skill.name} (${level})`,
@@ -134,7 +141,7 @@ export class SkillManager {
   /**
    * Loads a skill configuration by name.
    * If level is specified, only searches that level.
-   * If level is omitted, searches project-level first, then user-level.
+   * If level is omitted, searches in precedence order: project > user > extension > bundled.
    *
    * @param name - Name of the skill to load
    * @param level - Optional level to limit search to
@@ -165,7 +172,7 @@ export class SkillManager {
       return projectSkill;
     }
 
-    // Try user level first
+    // Try user level
     const userSkill = await this.findSkillByNameAtLevel(name, 'user');
     if (userSkill) {
       debugLogger.debug(`Found skill ${name} at user level`);
@@ -176,10 +183,19 @@ export class SkillManager {
     const extensionSkill = await this.findSkillByNameAtLevel(name, 'extension');
     if (extensionSkill) {
       debugLogger.debug(`Found skill ${name} at extension level`);
-    } else {
-      debugLogger.debug(`Skill ${name} not found at any level`);
+      return extensionSkill;
     }
-    return extensionSkill;
+
+    // Try bundled level (lowest precedence)
+    const bundledSkill = await this.findSkillByNameAtLevel(name, 'bundled');
+    if (bundledSkill) {
+      debugLogger.debug(`Found skill ${name} at bundled level`);
+    } else {
+      debugLogger.debug(
+        `Skill ${name} not found at any level (checked: project, user, extension, bundled)`,
+      );
+    }
+    return bundledSkill;
   }
 
   /**
@@ -227,7 +243,7 @@ export class SkillManager {
     const skillsCache = new Map<SkillLevel, SkillConfig[]>();
     this.parseErrors.clear();
 
-    const levels: SkillLevel[] = ['project', 'user', 'extension'];
+    const levels: SkillLevel[] = ['project', 'user', 'extension', 'bundled'];
     let totalSkills = 0;
 
     for (const level of levels) {
@@ -416,15 +432,24 @@ export class SkillManager {
    * @returns Absolute directory paths
    */
   getSkillsBaseDirs(level: SkillLevel): string[] {
-    const baseDirs =
-      level === 'project'
-        ? SKILL_PROVIDER_CONFIG_DIRS.map((v) =>
-            path.join(this.config.getProjectRoot(), v, SKILLS_CONFIG_DIR),
-          )
-        : SKILL_PROVIDER_CONFIG_DIRS.map((v) =>
-            path.join(os.homedir(), v, SKILLS_CONFIG_DIR),
-          );
-    return baseDirs;
+    switch (level) {
+      case 'project':
+        return SKILL_PROVIDER_CONFIG_DIRS.map((v) =>
+          path.join(this.config.getProjectRoot(), v, SKILLS_CONFIG_DIR),
+        );
+      case 'user':
+        return SKILL_PROVIDER_CONFIG_DIRS.map((v) =>
+          path.join(os.homedir(), v, SKILLS_CONFIG_DIR),
+        );
+      case 'bundled':
+        return [this.bundledSkillsDir];
+      case 'extension':
+        throw new Error(
+          'Extension skills do not have a base directory; they are loaded from active extensions.',
+        );
+      default:
+        throw new Error(`Unknown skill level: ${level as string}`);
+    }
   }
 
   /**
@@ -461,6 +486,20 @@ export class SkillManager {
       return skills;
     }
 
+    if (level === 'bundled') {
+      const bundledDir = this.bundledSkillsDir;
+      if (!fsSync.existsSync(bundledDir)) {
+        debugLogger.warn(
+          `Bundled skills directory not found: ${bundledDir}. This may indicate an incomplete installation.`,
+        );
+        return [];
+      }
+      debugLogger.debug(`Loading bundled skills from: ${bundledDir}`);
+      const skills = await this.loadSkillsFromDir(bundledDir, 'bundled');
+      debugLogger.debug(`Loaded ${skills.length} bundled skills`);
+      return skills;
+    }
+
     // Iterate provider directories in PROVIDER_CONFIG_DIRS order.
     // The first directory that contains a skill with a given name wins,
     // so the order defines implicit precedence (.qwen > .agent > .cursor > ...).
@@ -597,6 +636,9 @@ export class SkillManager {
     }
   }
 
+  // Only watch project and user skill directories for changes.
+  // Bundled skills are immutable (shipped with the package) and extension
+  // skills are managed by the extension system, so neither needs watching.
   private updateWatchersFromCache(): void {
     const watchTargets = new Set<string>(
       (['project', 'user'] as const)

packages/core/src/skills/types.ts

@@ -9,8 +9,9 @@
  * - 'project': Stored in `.qwen/skills/` within the project directory
  * - 'user': Stored in `~/.qwen/skills/` in the user's home directory
  * - 'extension': Provided by an installed extension
+ * - 'bundled': Built-in skills shipped with qwen-code
  */
-export type SkillLevel = 'project' | 'user' | 'extension';
+export type SkillLevel = 'project' | 'user' | 'extension' | 'bundled';
 
 /**
  * Core configuration for a skill as stored in SKILL.md files.