Skip to content

Commit 1359563

Browse files
authored
feat(skills): add bundled /review skill for out-of-the-box code review (#2348)
feat(skills): add bundled /review skill for out-of-the-box code review
1 parent f1ee463 commit 1359563

File tree

11 files changed

+524
-26
lines changed

11 files changed

+524
-26
lines changed

packages/cli/src/nonInteractiveCliCommands.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
} from '@qwen-code/qwen-code-core';
1515
import { CommandService } from './services/CommandService.js';
1616
import { BuiltinCommandLoader } from './services/BuiltinCommandLoader.js';
17+
import { BundledSkillLoader } from './services/BundledSkillLoader.js';
1718
import { FileCommandLoader } from './services/FileCommandLoader.js';
1819
import {
1920
CommandKind,
@@ -197,7 +198,7 @@ function filterCommandsForNonInteractive(
197198
allowedBuiltinCommandNames: Set<string>,
198199
): SlashCommand[] {
199200
return commands.filter((cmd) => {
200-
if (cmd.kind === CommandKind.FILE) {
201+
if (cmd.kind === CommandKind.FILE || cmd.kind === CommandKind.SKILL) {
201202
return true;
202203
}
203204

@@ -252,6 +253,7 @@ export const handleSlashCommand = async (
252253
// Load all commands to check if the command exists but is not allowed
253254
const allLoaders = [
254255
new BuiltinCommandLoader(config),
256+
new BundledSkillLoader(config),
255257
new FileCommandLoader(config),
256258
];
257259

@@ -366,8 +368,12 @@ export const getAvailableCommands = async (
366368
// Only load BuiltinCommandLoader if there are allowed built-in commands
367369
const loaders =
368370
allowedBuiltinSet.size > 0
369-
? [new BuiltinCommandLoader(config), new FileCommandLoader(config)]
370-
: [new FileCommandLoader(config)];
371+
? [
372+
new BuiltinCommandLoader(config),
373+
new BundledSkillLoader(config),
374+
new FileCommandLoader(config),
375+
]
376+
: [new BundledSkillLoader(config), new FileCommandLoader(config)];
371377

372378
const commandService = await CommandService.create(loaders, abortSignal);
373379
const commands = commandService.getCommands();
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/**
2+
* @license
3+
* Copyright 2025 Qwen
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import { describe, it, expect, vi, beforeEach } from 'vitest';
8+
import { BundledSkillLoader } from './BundledSkillLoader.js';
9+
import { CommandKind } from '../ui/commands/types.js';
10+
import type { Config, SkillConfig } from '@qwen-code/qwen-code-core';
11+
12+
function makeSkill(overrides: Partial<SkillConfig> = {}): SkillConfig {
13+
return {
14+
name: 'review',
15+
description: 'Review code changes',
16+
level: 'bundled',
17+
filePath: '/bundled/review/SKILL.md',
18+
body: 'You are an expert code reviewer.',
19+
...overrides,
20+
};
21+
}
22+
23+
describe('BundledSkillLoader', () => {
24+
let mockConfig: Config;
25+
let mockSkillManager: {
26+
listSkills: ReturnType<typeof vi.fn>;
27+
};
28+
29+
beforeEach(() => {
30+
vi.clearAllMocks();
31+
mockSkillManager = {
32+
listSkills: vi.fn().mockResolvedValue([]),
33+
};
34+
mockConfig = {
35+
getSkillManager: vi.fn().mockReturnValue(mockSkillManager),
36+
} as unknown as Config;
37+
});
38+
39+
const signal = new AbortController().signal;
40+
41+
it('should return empty array when config is null', async () => {
42+
const loader = new BundledSkillLoader(null);
43+
const commands = await loader.loadCommands(signal);
44+
expect(commands).toEqual([]);
45+
});
46+
47+
it('should return empty array when SkillManager is not available', async () => {
48+
const config = {
49+
getSkillManager: vi.fn().mockReturnValue(null),
50+
} as unknown as Config;
51+
const loader = new BundledSkillLoader(config);
52+
const commands = await loader.loadCommands(signal);
53+
expect(commands).toEqual([]);
54+
});
55+
56+
it('should load bundled skills as slash commands', async () => {
57+
const skill = makeSkill();
58+
mockSkillManager.listSkills.mockResolvedValue([skill]);
59+
60+
const loader = new BundledSkillLoader(mockConfig);
61+
const commands = await loader.loadCommands(signal);
62+
63+
expect(commands).toHaveLength(1);
64+
expect(commands[0].name).toBe('review');
65+
expect(commands[0].description).toBe('Review code changes');
66+
expect(commands[0].kind).toBe(CommandKind.SKILL);
67+
expect(mockSkillManager.listSkills).toHaveBeenCalledWith({
68+
level: 'bundled',
69+
});
70+
});
71+
72+
it('should submit skill body as prompt without args', async () => {
73+
const skill = makeSkill();
74+
mockSkillManager.listSkills.mockResolvedValue([skill]);
75+
76+
const loader = new BundledSkillLoader(mockConfig);
77+
const commands = await loader.loadCommands(signal);
78+
const result = await commands[0].action!(
79+
{ invocation: { raw: '/review', args: '' } } as never,
80+
'',
81+
);
82+
83+
expect(result).toEqual({
84+
type: 'submit_prompt',
85+
content: [{ text: 'You are an expert code reviewer.' }],
86+
});
87+
});
88+
89+
it('should append raw invocation when args are provided', async () => {
90+
const skill = makeSkill();
91+
mockSkillManager.listSkills.mockResolvedValue([skill]);
92+
93+
const loader = new BundledSkillLoader(mockConfig);
94+
const commands = await loader.loadCommands(signal);
95+
const result = await commands[0].action!(
96+
{ invocation: { raw: '/review 123', args: '123' } } as never,
97+
'123',
98+
);
99+
100+
expect(result).toEqual({
101+
type: 'submit_prompt',
102+
content: [{ text: 'You are an expert code reviewer.\n\n/review 123' }],
103+
});
104+
});
105+
106+
it('should return empty array when listSkills throws', async () => {
107+
mockSkillManager.listSkills.mockRejectedValue(new Error('load failed'));
108+
109+
const loader = new BundledSkillLoader(mockConfig);
110+
const commands = await loader.loadCommands(signal);
111+
112+
expect(commands).toEqual([]);
113+
});
114+
115+
it('should load multiple bundled skills', async () => {
116+
const skills = [
117+
makeSkill({ name: 'review', description: 'Review code' }),
118+
makeSkill({ name: 'deploy', description: 'Deploy app' }),
119+
];
120+
mockSkillManager.listSkills.mockResolvedValue(skills);
121+
122+
const loader = new BundledSkillLoader(mockConfig);
123+
const commands = await loader.loadCommands(signal);
124+
125+
expect(commands).toHaveLength(2);
126+
expect(commands.map((c) => c.name)).toEqual(['review', 'deploy']);
127+
});
128+
});
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/**
2+
* @license
3+
* Copyright 2025 Qwen
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import type { Config } from '@qwen-code/qwen-code-core';
8+
import {
9+
createDebugLogger,
10+
appendToLastTextPart,
11+
} from '@qwen-code/qwen-code-core';
12+
import type { ICommandLoader } from './types.js';
13+
import type {
14+
SlashCommand,
15+
SlashCommandActionReturn,
16+
} from '../ui/commands/types.js';
17+
import { CommandKind } from '../ui/commands/types.js';
18+
19+
const debugLogger = createDebugLogger('BUNDLED_SKILL_LOADER');
20+
21+
/**
22+
* Loads bundled skills as slash commands, making them directly invocable
23+
* via /<skill-name> (e.g., /review).
24+
*/
25+
export class BundledSkillLoader implements ICommandLoader {
26+
constructor(private readonly config: Config | null) {}
27+
28+
async loadCommands(_signal: AbortSignal): Promise<SlashCommand[]> {
29+
const skillManager = this.config?.getSkillManager();
30+
if (!skillManager) {
31+
debugLogger.debug('SkillManager not available, skipping bundled skills');
32+
return [];
33+
}
34+
35+
try {
36+
const skills = await skillManager.listSkills({ level: 'bundled' });
37+
debugLogger.debug(
38+
`Loaded ${skills.length} bundled skill(s) as slash commands`,
39+
);
40+
41+
return skills.map((skill) => ({
42+
name: skill.name,
43+
description: skill.description,
44+
kind: CommandKind.SKILL,
45+
action: async (context, _args): Promise<SlashCommandActionReturn> => {
46+
const content = context.invocation?.args
47+
? appendToLastTextPart(
48+
[{ text: skill.body }],
49+
context.invocation.raw,
50+
)
51+
: [{ text: skill.body }];
52+
53+
return {
54+
type: 'submit_prompt',
55+
content,
56+
};
57+
},
58+
}));
59+
} catch (error) {
60+
debugLogger.error('Failed to load bundled skills:', error);
61+
return [];
62+
}
63+
}
64+
}

packages/cli/src/ui/commands/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ export enum CommandKind {
211211
BUILT_IN = 'built-in',
212212
FILE = 'file',
213213
MCP_PROMPT = 'mcp-prompt',
214+
SKILL = 'skill',
214215
}
215216

216217
export interface CommandCompletionItem {

packages/cli/src/ui/hooks/slashCommandProcessor.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import type { LoadedSettings } from '../../config/settings.js';
3131
import { type CommandContext, type SlashCommand } from '../commands/types.js';
3232
import { CommandService } from '../../services/CommandService.js';
3333
import { BuiltinCommandLoader } from '../../services/BuiltinCommandLoader.js';
34+
import { BundledSkillLoader } from '../../services/BundledSkillLoader.js';
3435
import { FileCommandLoader } from '../../services/FileCommandLoader.js';
3536
import { McpPromptLoader } from '../../services/McpPromptLoader.js';
3637
import { parseSlashCommand } from '../../utils/commands.js';
@@ -311,6 +312,7 @@ export const useSlashCommandProcessor = (
311312
const loaders = [
312313
new McpPromptLoader(config),
313314
new BuiltinCommandLoader(config),
315+
new BundledSkillLoader(config),
314316
new FileCommandLoader(config),
315317
];
316318
const commandService = await CommandService.create(
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
---
2+
name: review
3+
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>`.
4+
allowedTools:
5+
- task
6+
- run_shell_command
7+
- grep_search
8+
- read_file
9+
- glob
10+
---
11+
12+
# Code Review
13+
14+
You are an expert code reviewer. Your job is to review code changes and provide actionable feedback.
15+
16+
## Step 1: Determine what to review
17+
18+
Based on the arguments provided:
19+
20+
- **No arguments**: Review local uncommitted changes
21+
- Run `git diff` and `git diff --staged` to get all changes
22+
- If both diffs are empty, inform the user there are no changes to review and stop here — do not proceed to the review agents
23+
24+
- **PR number or URL** (e.g., `123` or `https://github.com/.../pull/123`):
25+
- Run `gh pr view <number>` to get PR details
26+
- Run `gh pr diff <number>` to get the diff
27+
28+
- **File path** (e.g., `src/foo.ts`):
29+
- Run `git diff HEAD -- <file>` to get recent changes
30+
- If no diff, read the file and review its current state
31+
32+
## Step 2: Parallel multi-dimensional review
33+
34+
Launch **four parallel review agents** to analyze the changes from different angles. Each agent should focus exclusively on its dimension.
35+
36+
### Agent 1: Correctness & Security
37+
38+
Focus areas:
39+
40+
- Logic errors and edge cases
41+
- Null/undefined handling
42+
- Race conditions and concurrency issues
43+
- Security vulnerabilities (injection, XSS, SSRF, path traversal, etc.)
44+
- Type safety issues
45+
- Error handling gaps
46+
47+
### Agent 2: Code Quality
48+
49+
Focus areas:
50+
51+
- Code style consistency with the surrounding codebase
52+
- Naming conventions (variables, functions, classes)
53+
- Code duplication and opportunities for reuse
54+
- Over-engineering or unnecessary abstraction
55+
- Missing or misleading comments
56+
- Dead code
57+
58+
### Agent 3: Performance & Efficiency
59+
60+
Focus areas:
61+
62+
- Performance bottlenecks (N+1 queries, unnecessary loops, etc.)
63+
- Memory leaks or excessive memory usage
64+
- Unnecessary re-renders (for UI code)
65+
- Inefficient algorithms or data structures
66+
- Missing caching opportunities
67+
- Bundle size impact
68+
69+
### Agent 4: Undirected Audit
70+
71+
No preset dimension. Review the code with a completely fresh perspective to catch issues the other three agents may miss.
72+
Focus areas:
73+
74+
- Business logic soundness and correctness of assumptions
75+
- Boundary interactions between modules or services
76+
- Implicit assumptions that may break under different conditions
77+
- Unexpected side effects or hidden coupling
78+
- Anything else that looks off — trust your instincts
79+
80+
## Step 3: Aggregate and present findings
81+
82+
Combine results from all four agents into a single, well-organized review. Use this format:
83+
84+
### Summary
85+
86+
A 1-2 sentence overview of the changes and overall assessment.
87+
88+
### Findings
89+
90+
Use severity levels:
91+
92+
- **Critical** — Must fix before merging. Bugs, security issues, data loss risks.
93+
- **Suggestion** — Recommended improvement. Better patterns, clearer code, potential issues.
94+
- **Nice to have** — Optional optimization. Minor style tweaks, small performance gains.
95+
96+
For each finding, include:
97+
98+
1. **File and line reference** (e.g., `src/foo.ts:42`)
99+
2. **What's wrong** — Clear description of the issue
100+
3. **Why it matters** — Impact if not addressed
101+
4. **Suggested fix** — Concrete code suggestion when possible
102+
103+
### Verdict
104+
105+
One of:
106+
107+
- **Approve** — No critical issues, good to merge
108+
- **Request changes** — Has critical issues that need fixing
109+
- **Comment** — Has suggestions but no blockers
110+
111+
## Guidelines
112+
113+
- Be specific and actionable. Avoid vague feedback like "could be improved."
114+
- Reference the existing codebase conventions — don't impose external style preferences.
115+
- Focus on the diff, not pre-existing issues in unchanged code.
116+
- Keep the review concise. Don't repeat the same point for every occurrence.
117+
- When suggesting a fix, show the actual code change.
118+
- Flag any exposed secrets, credentials, API keys, or tokens in the diff as **Critical**.

packages/core/src/skills/index.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,13 @@
1111
* users to define reusable skill configurations that can be loaded by the
1212
* model via a dedicated Skills tool.
1313
*
14-
* Skills are stored as directories in `.qwen/skills/` (project-level) or
15-
* `~/.qwen/skills/` (user-level), with each directory containing a SKILL.md
16-
* file with YAML frontmatter for metadata.
14+
* Skills are stored as directories containing a SKILL.md file with YAML
15+
* frontmatter for metadata. They can be loaded from four levels
16+
* (precedence: project > user > extension > bundled):
17+
* - Project-level: `.qwen/skills/`
18+
* - User-level: `~/.qwen/skills/`
19+
* - Extension-level: provided by installed extensions
20+
* - Bundled: built-in skills shipped with qwen-code
1721
*/
1822

1923
// Core types and interfaces

0 commit comments

Comments
 (0)