feat(cli): add skill_lookup MCP for non-native agents (#1035)

* test: reproduce issue #752

* fix: expose skill lookup MCP tool (closes #752)

* test: cover ACP skill lookup instructions

* fix: inject ACP skill lookup instruction

* test: narrow skill lookup auto-approval

* fix: restrict skill lookup auto-approval

* test: cover exact skill lookup tool names
This commit is contained in:
SSU-WEI HUANG
2026-07-16 12:31:36 +08:00
committed by GitHub
parent e342d97177
commit f457156bd1
25 changed files with 941 additions and 90 deletions
@@ -0,0 +1,38 @@
import { describe, expect, it } from 'vitest'
import { resolveToolAutoApprovalDecision } from './BasePermissionHandler'
describe('resolveToolAutoApprovalDecision skill_lookup', () => {
it.each([
'skill_lookup',
'hapi_skill_lookup',
'happy__skill_lookup',
'mcp__hapi__skill_lookup'
])('auto-approves the exact read-only HAPI tool name %s', (toolName) => {
expect(resolveToolAutoApprovalDecision(
'default',
toolName,
'call-1'
)).toBe('approved')
})
it('does not approve another tool solely from a skill-looking call id', () => {
expect(resolveToolAutoApprovalDecision(
'default',
'dangerous_tool',
'skill_lookup-forged-id'
)).toBeNull()
})
it('does not approve another tool whose name only contains skill_lookup', () => {
expect(resolveToolAutoApprovalDecision(
'default',
'skill_lookup_write_file',
'call-1'
)).toBeNull()
expect(resolveToolAutoApprovalDecision(
'default',
'dangerous_skill_lookup',
'call-2'
)).toBeNull()
})
})
@@ -26,6 +26,12 @@ const AUTO_APPROVE_TOOL_NAME_HINTS = [
'think',
'save_memory'
];
const AUTO_APPROVE_EXACT_TOOL_NAMES = new Set([
'skill_lookup',
'hapi_skill_lookup',
'happy__skill_lookup',
'mcp__hapi__skill_lookup'
]);
const AUTO_APPROVE_TOOL_ID_HINTS = ['change_title', 'save_memory'];
const AUTO_APPROVE_WRITE_TOOL_HINTS = ['write', 'edit', 'create', 'delete', 'patch', 'fs-edit'];
@@ -45,7 +51,10 @@ export function resolveToolAutoApprovalDecision(
const lowerId = toolCallId.toLowerCase();
const decisionForMode: AutoApprovalDecision = mode === 'yolo' ? 'approved_for_session' : 'approved';
if (rules.alwaysToolNameHints.some((name) => lowerTool.includes(name))) {
if (
AUTO_APPROVE_EXACT_TOOL_NAMES.has(lowerTool)
|| rules.alwaysToolNameHints.some((name) => lowerTool.includes(name))
) {
return decisionForMode;
}
@@ -0,0 +1,2 @@
export const SKILL_LOOKUP_INSTRUCTION =
'When a user message starts with "$name", call HAPI\'s skill_lookup tool with "name" (without "$") before acting.'
+80 -2
View File
@@ -1,8 +1,8 @@
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'
import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { listSkills } from './skills'
import { listSkills, MAX_SKILL_FILE_BYTES, resolveSkill } from './skills'
async function writeSkill(skillDir: string, name: string, description: string): Promise<void> {
await mkdir(skillDir, { recursive: true })
@@ -327,4 +327,82 @@ describe('listSkills', () => {
description: 'Local shared skill'
})
})
it('resolves the same nearest project skill selected by listSkills', async () => {
const repoRoot = join(sandboxDir, 'repo')
const workingDirectory = join(repoRoot, 'apps', 'web')
await mkdir(join(repoRoot, '.git'), { recursive: true })
await writeSkill(join(homeDir, '.agents', 'skills', 'shared'), 'shared', 'User shared skill')
await writeSkill(join(repoRoot, '.agents', 'skills', 'shared'), 'shared', 'Repo shared skill')
await writeSkill(join(workingDirectory, '.agents', 'skills', 'shared'), 'shared', 'Local shared skill')
await expect(resolveSkill('shared', workingDirectory, { flavor: 'opencode' })).resolves.toEqual({
name: 'shared',
description: 'Local shared skill',
body: '# shared'
})
})
it('resolves a user skill when no project duplicate exists', async () => {
await writeSkill(join(homeDir, '.agents', 'skills', 'user-only'), 'user-only', 'User skill')
await expect(resolveSkill('user-only', undefined, { flavor: 'kimi' })).resolves.toEqual({
name: 'user-only',
description: 'User skill',
body: '# user-only'
})
})
it('returns null for an unknown exact skill name', async () => {
await writeSkill(join(homeDir, '.agents', 'skills', 'known'), 'known', 'Known skill')
await expect(resolveSkill('missing')).resolves.toBeNull()
})
it('preserves support for a symlinked SKILL.md file', async () => {
const source = join(sandboxDir, 'shared-skill.md')
const skillDir = join(homeDir, '.agents', 'skills', 'linked')
await mkdir(skillDir, { recursive: true })
await writeFile(source, [
'---',
'name: linked',
'description: Linked skill',
'---',
'',
'# Linked body'
].join('\n'))
await symlink(source, join(skillDir, 'SKILL.md'))
await expect(resolveSkill('linked')).resolves.toEqual({
name: 'linked',
description: 'Linked skill',
body: '# Linked body'
})
})
it('rejects path traversal instead of treating it as a skill path', async () => {
await expect(resolveSkill('../settings.json')).rejects.toThrow('Invalid skill name')
await expect(resolveSkill('nested/skill')).rejects.toThrow('Invalid skill name')
await expect(resolveSkill('nested\\skill')).rejects.toThrow('Invalid skill name')
})
it('rejects a skill file that is too large to place in model context', async () => {
const skillDir = join(homeDir, '.agents', 'skills', 'huge')
await mkdir(skillDir, { recursive: true })
await writeFile(join(skillDir, 'SKILL.md'), [
'---',
'name: huge',
'description: Huge skill',
'---',
'',
'x'.repeat(MAX_SKILL_FILE_BYTES)
].join('\n'))
await expect(listSkills()).resolves.toContainEqual({
name: 'huge',
description: 'Huge skill'
})
await expect(resolveSkill('huge')).rejects.toThrow('Skill is too large to load')
})
})
+95 -10
View File
@@ -1,4 +1,4 @@
import { access, readdir, readFile } from 'fs/promises';
import { access, open, readdir, readFile } from 'fs/promises';
import { basename, dirname, join, resolve } from 'path';
import { homedir } from 'os';
import { parse as parseYaml } from 'yaml';
@@ -8,6 +8,12 @@ export interface SkillSummary {
description?: string;
}
export interface ResolvedSkill extends SkillSummary {
body: string;
}
export const MAX_SKILL_FILE_BYTES = 128 * 1024;
export interface ListSkillsRequest {
flavor?: string;
}
@@ -28,6 +34,10 @@ type InstalledPluginsFile = {
plugins?: Record<string, InstalledPlugin[]>;
};
type DiscoveredSkill = ResolvedSkill & {
fileSize: number;
};
function getHomeDirectory(): string {
return process.env.HOME ?? process.env.USERPROFILE ?? homedir();
}
@@ -182,18 +192,51 @@ async function listTopLevelSkillDirs(skillsRoot: string, options: { includeCodex
}
}
async function readSkillsFromDirs(skillDirs: string[]): Promise<SkillSummary[]> {
const skills = await Promise.all(skillDirs.map(async (dir): Promise<SkillSummary | null> => {
const filePath = join(dir, 'SKILL.md');
async function readSkillFile(filePath: string): Promise<{ content: string; fileSize: number } | null> {
try {
const file = await open(filePath, 'r');
try {
const fileContent = await readFile(filePath, 'utf-8');
return extractSkillSummary(dir, fileContent);
} catch {
const info = await file.stat();
if (!info.isFile()) {
return null;
}
const bytesToRead = Math.min(info.size, MAX_SKILL_FILE_BYTES + 1);
const buffer = Buffer.alloc(bytesToRead);
const { bytesRead } = await file.read(buffer, 0, bytesToRead, 0);
return {
content: buffer.subarray(0, bytesRead).toString('utf-8'),
fileSize: info.size
};
} finally {
await file.close();
}
} catch {
return null;
}
}
async function readSkillsFromDirs(skillDirs: string[]): Promise<DiscoveredSkill[]> {
const skills = await Promise.all(skillDirs.map(async (dir): Promise<DiscoveredSkill | null> => {
const filePath = join(dir, 'SKILL.md');
const skillFile = await readSkillFile(filePath);
if (!skillFile) {
return null;
}
const summary = extractSkillSummary(dir, skillFile.content);
if (!summary) {
return null;
}
return {
...summary,
body: parseFrontmatter(skillFile.content).body,
fileSize: skillFile.fileSize
};
}));
return skills.filter((skill): skill is SkillSummary => skill !== null);
return skills.filter((skill): skill is DiscoveredSkill => skill !== null);
}
function shouldIncludeCodexSystem(root: string, flavor: string): boolean {
@@ -230,7 +273,7 @@ async function listPluginCacheSkillsRoots(flavor?: string): Promise<string[]> {
.map((installPath) => join(installPath, 'skills'));
}
export async function listSkills(workingDirectory?: string, options: { flavor?: string } = {}): Promise<SkillSummary[]> {
async function discoverSkills(workingDirectory?: string, options: { flavor?: string } = {}): Promise<DiscoveredSkill[]> {
const flavor = normalizeFlavor(options.flavor);
const projectRoots = await listProjectSkillsRoots(workingDirectory, flavor);
const userRoots = getUserSkillsRoots(flavor);
@@ -251,7 +294,7 @@ export async function listSkills(workingDirectory?: string, options: { flavor?:
readSkillsFromDirs(adminSkillDirs),
]);
const dedupedSkills = new Map<string, SkillSummary>();
const dedupedSkills = new Map<string, DiscoveredSkill>();
for (const skill of [
...projectSkills,
...userSkills,
@@ -265,3 +308,45 @@ export async function listSkills(workingDirectory?: string, options: { flavor?:
return [...dedupedSkills.values()].sort((a, b) => a.name.localeCompare(b.name));
}
export async function listSkills(workingDirectory?: string, options: { flavor?: string } = {}): Promise<SkillSummary[]> {
const skills = await discoverSkills(workingDirectory, options);
return skills.map(({ name, description }) => ({ name, description }));
}
function validateSkillName(name: string): string {
const trimmed = name.trim();
if (
!trimmed
|| trimmed.length > 128
|| trimmed === '.'
|| trimmed === '..'
|| trimmed.includes('/')
|| trimmed.includes('\\')
|| trimmed.includes('\0')
) {
throw new Error('Invalid skill name');
}
return trimmed;
}
export async function resolveSkill(
name: string,
workingDirectory?: string,
options: { flavor?: string } = {}
): Promise<ResolvedSkill | null> {
const skillName = validateSkillName(name);
const skills = await discoverSkills(workingDirectory, options);
const skill = skills.find((candidate) => candidate.name === skillName);
if (!skill) {
return null;
}
if (skill.fileSize > MAX_SKILL_FILE_BYTES) {
throw new Error(`Skill is too large to load (maximum ${MAX_SKILL_FILE_BYTES} bytes)`);
}
return {
name: skill.name,
description: skill.description,
body: skill.body
};
}