Merge pull request #2932 from QwenLM/feat/review-skill-improvements

feat(review): enhance /review with deterministic analysis, autofix, and security hardening

packages/cli/src/services/BundledSkillLoader.test.ts

@@ -34,6 +34,7 @@ describe('BundledSkillLoader', () => {
     mockConfig = {
       getSkillManager: vi.fn().mockReturnValue(mockSkillManager),
       isCronEnabled: vi.fn().mockReturnValue(false),
+      getModel: vi.fn().mockReturnValue(undefined),
     } as unknown as Config;
   });
 
@@ -127,6 +128,115 @@ describe('BundledSkillLoader', () => {
     expect(commands.map((c) => c.name)).toEqual(['review', 'deploy']);
   });
 
+  it('should resolve {{model}} template variable in skill body', async () => {
+    const skill = makeSkill({
+      body: 'Review by {{model}} via Qwen Code',
+    });
+    mockSkillManager.listSkills.mockResolvedValue([skill]);
+    (mockConfig.getModel as ReturnType<typeof vi.fn>).mockReturnValue(
+      'qwen3-coder',
+    );
+
+    const loader = new BundledSkillLoader(mockConfig);
+    const commands = await loader.loadCommands(signal);
+    const result = await commands[0].action!(
+      { invocation: { raw: '/review', args: '' } } as never,
+      '',
+    );
+
+    expect(result).toEqual({
+      type: 'submit_prompt',
+      content: [
+        {
+          text: 'YOUR_MODEL_ID="qwen3-coder"\n\nReview by qwen3-coder via Qwen Code',
+        },
+      ],
+    });
+  });
+
+  it('should use empty string for {{model}} when getModel returns undefined', async () => {
+    const skill = makeSkill({
+      body: 'Review by {{model}}',
+    });
+    mockSkillManager.listSkills.mockResolvedValue([skill]);
+    // getModel returns undefined (default mock behavior)
+
+    const loader = new BundledSkillLoader(mockConfig);
+    const commands = await loader.loadCommands(signal);
+    const result = await commands[0].action!(
+      { invocation: { raw: '/review', args: '' } } as never,
+      '',
+    );
+
+    expect(result).toEqual({
+      type: 'submit_prompt',
+      content: [{ text: 'Review by ' }],
+    });
+  });
+
+  it('should resolve {{model}} when args are provided', async () => {
+    const skill = makeSkill({
+      body: 'Review by {{model}}',
+    });
+    mockSkillManager.listSkills.mockResolvedValue([skill]);
+    (mockConfig.getModel as ReturnType<typeof vi.fn>).mockReturnValue(
+      'qwen3-coder',
+    );
+
+    const loader = new BundledSkillLoader(mockConfig);
+    const commands = await loader.loadCommands(signal);
+    const result = await commands[0].action!(
+      { invocation: { raw: '/review 123', args: '123' } } as never,
+      '123',
+    );
+
+    expect(result).toEqual({
+      type: 'submit_prompt',
+      content: [
+        {
+          text: 'YOUR_MODEL_ID="qwen3-coder"\n\nReview by qwen3-coder\n\n/review 123',
+        },
+      ],
+    });
+  });
+
+  it('should use empty string for {{model}} when getModel returns empty string', async () => {
+    const skill = makeSkill({
+      body: 'Review by {{model}}',
+    });
+    mockSkillManager.listSkills.mockResolvedValue([skill]);
+    (mockConfig.getModel as ReturnType<typeof vi.fn>).mockReturnValue('');
+
+    const loader = new BundledSkillLoader(mockConfig);
+    const commands = await loader.loadCommands(signal);
+    const result = await commands[0].action!(
+      { invocation: { raw: '/review', args: '' } } as never,
+      '',
+    );
+
+    expect(result).toEqual({
+      type: 'submit_prompt',
+      content: [{ text: 'Review by ' }],
+    });
+  });
+
+  it('should not modify skill body without {{model}} template', async () => {
+    const skill = makeSkill({ body: 'No template here' });
+    mockSkillManager.listSkills.mockResolvedValue([skill]);
+
+    const loader = new BundledSkillLoader(mockConfig);
+    const commands = await loader.loadCommands(signal);
+    const result = await commands[0].action!(
+      { invocation: { raw: '/review', args: '' } } as never,
+      '',
+    );
+
+    expect(result).toEqual({
+      type: 'submit_prompt',
+      content: [{ text: 'No template here' }],
+    });
+  });
+
   it('should hide skills with cron allowedTools when cron is disabled', async () => {
     const skills = [
       makeSkill({ name: 'review', description: 'Review code' }),

packages/cli/src/services/BundledSkillLoader.ts

@@ -59,12 +59,22 @@ export class BundledSkillLoader implements ICommandLoader {
         description: skill.description,
         kind: CommandKind.SKILL,
         action: async (context, _args): Promise<SlashCommandActionReturn> => {
+          // Resolve template variables in skill body
+          let body = skill.body;
+          const modelId = this.config?.getModel()?.trim() || '';
+          if (body.includes('{{model}}') || body.includes('YOUR_MODEL_ID')) {
+            body = body.replaceAll('{{model}}', modelId);
+            body = body.replaceAll('YOUR_MODEL_ID', modelId);
+            // Prepend model identity as a top-level declaration so the LLM
+            // cannot miss it even if it doesn't copy the template exactly.
+            if (modelId) {
+              body = `YOUR_MODEL_ID="${modelId}"\n\n${body}`;
+            }
+          }
+
           const content = context.invocation?.args
-            ? appendToLastTextPart(
-                [{ text: skill.body }],
-                context.invocation.raw,
-              )
-            : [{ text: skill.body }];
+            ? appendToLastTextPart([{ text: body }], context.invocation.raw)
+            : [{ text: body }];
 
           return {
             type: 'submit_prompt',