From f457156bd1f4d502ff6cb17f600a96849ddb9f17 Mon Sep 17 00:00:00 2001 From: SSU-WEI HUANG Date: Thu, 16 Jul 2026 12:31:36 +0800 Subject: [PATCH] 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 --- cli/src/agent/runners/runAgentSession.test.ts | 82 +++++++++-- cli/src/agent/runners/runAgentSession.ts | 25 +++- cli/src/claude/utils/startHappyServer.test.ts | 113 +++++++++++++++ cli/src/claude/utils/startHappyServer.ts | 68 ++++++++- cli/src/codex/happyMcpStdioBridge.test.ts | 87 ++++++++++++ cli/src/codex/happyMcpStdioBridge.ts | 131 ++++++++++++------ .../codex/utils/buildHapiMcpBridge.test.ts | 72 ++++++++++ cli/src/codex/utils/buildHapiMcpBridge.ts | 31 ++++- .../cursor/cursorAcpRemoteLauncher.test.ts | 8 +- cli/src/cursor/cursorAcpRemoteLauncher.ts | 14 +- cli/src/grok/grokRemoteLauncher.test.ts | 3 + cli/src/grok/grokRemoteLauncher.ts | 4 +- cli/src/grok/utils/systemPrompt.ts | 4 +- cli/src/kimi/kimiRemoteLauncher.test.ts | 84 +++++++++++ cli/src/kimi/kimiRemoteLauncher.ts | 14 +- .../permission/BasePermissionHandler.test.ts | 38 +++++ .../permission/BasePermissionHandler.ts | 11 +- .../modules/common/skillLookupInstruction.ts | 2 + cli/src/modules/common/skills.test.ts | 82 ++++++++++- cli/src/modules/common/skills.ts | 105 ++++++++++++-- cli/src/opencode/opencodeLocalLauncher.ts | 4 +- .../opencode/opencodeRemoteLauncher.test.ts | 13 ++ cli/src/opencode/opencodeRemoteLauncher.ts | 4 +- cli/src/opencode/utils/systemPrompt.test.ts | 30 ++++ cli/src/opencode/utils/systemPrompt.ts | 2 + 25 files changed, 941 insertions(+), 90 deletions(-) create mode 100644 cli/src/claude/utils/startHappyServer.test.ts create mode 100644 cli/src/codex/happyMcpStdioBridge.test.ts create mode 100644 cli/src/codex/utils/buildHapiMcpBridge.test.ts create mode 100644 cli/src/kimi/kimiRemoteLauncher.test.ts create mode 100644 cli/src/modules/common/permission/BasePermissionHandler.test.ts create mode 100644 cli/src/modules/common/skillLookupInstruction.ts create mode 100644 cli/src/opencode/utils/systemPrompt.test.ts diff --git a/cli/src/agent/runners/runAgentSession.test.ts b/cli/src/agent/runners/runAgentSession.test.ts index e2f34f1c..ef9d4ca1 100644 --- a/cli/src/agent/runners/runAgentSession.test.ts +++ b/cli/src/agent/runners/runAgentSession.test.ts @@ -7,7 +7,12 @@ const harness = vi.hoisted(() => ({ cancelPrompt: vi.fn(async () => {}), cancelAll: vi.fn(async () => {}), stopServer: vi.fn(), - disconnect: vi.fn(async () => {}) + disconnect: vi.fn(async () => {}), + prompts: [] as unknown[][], + startHappyServerOptions: null as unknown, + bridgeArgs: [] as string[], + newSessionOptions: null as unknown, + killHandler: null as null | (() => Promise) })) vi.mock('@/agent/sessionFactory', () => ({ @@ -38,8 +43,12 @@ vi.mock('@/agent/AgentRegistry', () => ({ AgentRegistry: { create: vi.fn(() => ({ initialize: vi.fn(async () => {}), - newSession: vi.fn(async () => 'agent-session-1'), - prompt: vi.fn(async () => { + newSession: vi.fn(async (options: unknown) => { + harness.newSessionOptions = options + return 'agent-session-1' + }), + prompt: vi.fn(async (_sessionId: string, content: unknown[]) => { + harness.prompts.push(content) if (harness.promptError) { throw harness.promptError } @@ -61,18 +70,27 @@ vi.mock('@/agent/permissionAdapter', () => ({ })) vi.mock('@/claude/utils/startHappyServer', () => ({ - startHappyServer: vi.fn(async () => ({ - url: 'http://127.0.0.1:1234', - stop: harness.stopServer - })) + startHappyServer: vi.fn(async (_session: unknown, options: unknown) => { + harness.startHappyServerOptions = options + return { + url: 'http://127.0.0.1:1234', + toolNames: ['change_title', 'display_image', 'skill_lookup'], + stop: harness.stopServer + } + }) })) vi.mock('@/utils/spawnHappyCLI', () => ({ - getHappyCliCommand: vi.fn(() => ({ command: 'hapi', args: [], env: [] })) + getHappyCliCommand: vi.fn((args: string[]) => { + harness.bridgeArgs = args + return { command: 'hapi', args, env: [] } + }) })) vi.mock('@/claude/registerKillSessionHandler', () => ({ - registerKillSessionHandler: vi.fn() + registerKillSessionHandler: vi.fn((_manager: unknown, handler: () => Promise) => { + harness.killHandler = handler + }) })) vi.mock('@/utils/invokedCwd', () => ({ @@ -101,6 +119,11 @@ describe('runAgentSession', () => { harness.cancelAll.mockClear() harness.stopServer.mockClear() harness.disconnect.mockClear() + harness.prompts = [] + harness.startHappyServerOptions = null + harness.bridgeArgs = [] + harness.newSessionOptions = null + harness.killHandler = null }) it('reports unhandled ACP runner failures as error, not completed', async () => { @@ -120,4 +143,45 @@ describe('runAgentSession', () => { expect(harness.sendSessionDeath).toHaveBeenCalledWith('error') expect(harness.sendSessionDeath).not.toHaveBeenCalledWith('completed') }) + + it('enables skill lookup and injects its instruction only on the first prompt', async () => { + const running = runAgentSession({ agentType: 'acp' }) + await vi.waitFor(() => expect(harness.userMessageHandler).not.toBeNull()) + + harness.userMessageHandler?.({ content: { text: 'first', attachments: [] } }, 'local-1') + await vi.waitFor(() => expect(harness.prompts).toHaveLength(1)) + harness.userMessageHandler?.({ content: { text: 'second', attachments: [] } }, 'local-2') + await vi.waitFor(() => expect(harness.prompts).toHaveLength(2)) + + await harness.killHandler?.() + await running + + expect(harness.startHappyServerOptions).toEqual({ + skillLookup: { + workingDirectory: '/tmp/project', + flavor: 'acp' + } + }) + expect(harness.bridgeArgs).toEqual([ + 'mcp', + '--url', + 'http://127.0.0.1:1234', + '--tools', + 'change_title,display_image,skill_lookup' + ]) + expect(harness.newSessionOptions).toMatchObject({ + cwd: '/tmp/project', + mcpServers: [{ + name: 'happy', + command: 'hapi', + args: harness.bridgeArgs + }] + }) + + const firstPrompt = JSON.stringify(harness.prompts[0]) + const secondPrompt = JSON.stringify(harness.prompts[1]) + expect(firstPrompt).toContain('$name') + expect(firstPrompt).toContain('skill_lookup') + expect(secondPrompt).not.toContain('skill_lookup') + }) }) diff --git a/cli/src/agent/runners/runAgentSession.ts b/cli/src/agent/runners/runAgentSession.ts index bd1ccb43..9c9d6b68 100644 --- a/cli/src/agent/runners/runAgentSession.ts +++ b/cli/src/agent/runners/runAgentSession.ts @@ -16,6 +16,7 @@ import { PermissionModeSchema } from '@hapi/protocol/schemas'; import { isPermissionModeAllowedForFlavor } from '@hapi/protocol'; import { RPC_METHODS } from '@hapi/protocol/rpcMethods'; import type { SessionEndReason } from '@hapi/protocol'; +import { SKILL_LOOKUP_INSTRUCTION } from '@/modules/common/skillLookupInstruction'; function emitReadyIfIdle(props: { queueSize: () => number; @@ -70,8 +71,19 @@ export async function runAgentSession(opts: { const permissionAdapter = new PermissionAdapter(session, backend, () => currentPermissionMode); - const happyServer = await startHappyServer(session); - const bridgeCommand = getHappyCliCommand(['mcp', '--url', happyServer.url]); + const happyServer = await startHappyServer(session, { + skillLookup: { + workingDirectory, + flavor: opts.agentType + } + }); + const bridgeCommand = getHappyCliCommand([ + 'mcp', + '--url', + happyServer.url, + '--tools', + happyServer.toolNames.join(',') + ]); const mcpServers = [ { name: 'happy', @@ -89,6 +101,7 @@ export async function runAgentSession(opts: { let thinking = false; let shouldExit = false; let waitAbortController: AbortController | null = null; + let skillLookupInstructionSent = false; const syncKeepAlive = () => { session.keepAlive(thinking, 'remote', { @@ -167,9 +180,15 @@ export async function runAgentSession(opts: { continue; } + let messageText = batch.message; + if (!skillLookupInstructionSent && !messageText.trimStart().startsWith('/')) { + messageText = `${SKILL_LOOKUP_INSTRUCTION}\n\n${messageText}`; + skillLookupInstructionSent = true; + } + const promptContent: PromptContent[] = [{ type: 'text', - text: batch.message + text: messageText }]; thinking = true; diff --git a/cli/src/claude/utils/startHappyServer.test.ts b/cli/src/claude/utils/startHappyServer.test.ts new file mode 100644 index 00000000..55df63b0 --- /dev/null +++ b/cli/src/claude/utils/startHappyServer.test.ts @@ -0,0 +1,113 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { Client } from '@modelcontextprotocol/sdk/client/index.js' +import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import type { ApiSessionClient } from '@/api/apiSession' +import { startHappyServer } from './startHappyServer' + +type ToolResult = { + content?: Array<{ type: string; text?: string }> + isError?: boolean +} + +describe('startHappyServer skill_lookup', () => { + const originalHome = process.env.HOME + let sandboxDir: string + let workingDirectory: string + let client: Client | null + let stopServer: (() => void) | null + + beforeEach(async () => { + sandboxDir = await mkdtemp(join(tmpdir(), 'hapi-skill-mcp-')) + workingDirectory = join(sandboxDir, 'repo') + process.env.HOME = join(sandboxDir, 'home') + await mkdir(join(workingDirectory, '.git'), { recursive: true }) + await mkdir(process.env.HOME, { recursive: true }) + client = null + stopServer = null + }) + + afterEach(async () => { + await client?.close() + stopServer?.() + if (originalHome === undefined) { + delete process.env.HOME + } else { + process.env.HOME = originalHome + } + await rm(sandboxDir, { recursive: true, force: true }) + }) + + async function connect(enableSkillLookup = true): Promise { + const sessionClient = { + updateMetadata: vi.fn(), + sendAgentMessage: vi.fn(), + sendClaudeSessionMessage: vi.fn() + } as unknown as ApiSessionClient + const server = await startHappyServer(sessionClient, enableSkillLookup + ? { + skillLookup: { + workingDirectory, + flavor: 'opencode' + } + } + : {}) + stopServer = server.stop + + client = new Client( + { name: 'hapi-skill-lookup-test', version: '1.0.0' }, + { capabilities: {} } + ) + await client.connect(new StreamableHTTPClientTransport(new URL(server.url))) + return client + } + + it('returns a discovered SKILL.md body', async () => { + const skillDir = join(workingDirectory, '.agents', 'skills', 'review') + await mkdir(skillDir, { recursive: true }) + await writeFile(join(skillDir, 'SKILL.md'), [ + '---', + 'name: review', + 'description: Review changes safely', + '---', + '', + '# Review instructions', + '', + 'Inspect the diff before editing.' + ].join('\n')) + + const mcp = await connect() + const result = await mcp.callTool({ + name: 'skill_lookup', + arguments: { name: 'review' } + }) as ToolResult + + expect(result.isError).toBe(false) + expect(result.content?.[0]?.text).toContain('Skill: review') + expect(result.content?.[0]?.text).toContain('Description: Review changes safely') + expect(result.content?.[0]?.text).toContain('# Review instructions') + }) + + it('returns a tool error for an unknown skill', async () => { + const mcp = await connect() + const result = await mcp.callTool({ + name: 'skill_lookup', + arguments: { name: 'missing' } + }) as ToolResult + + expect(result.isError).toBe(true) + expect(result.content?.[0]?.text).toContain('Skill not found: missing') + }) + + it('does not expose the fallback tool to native-skill sessions', async () => { + const mcp = await connect(false) + const tools = await mcp.listTools() + + expect(tools.tools.map((tool) => tool.name)).toEqual([ + 'change_title', + 'display_image' + ]) + }) +}) diff --git a/cli/src/claude/utils/startHappyServer.ts b/cli/src/claude/utils/startHappyServer.ts index b2001a57..28bde1c4 100644 --- a/cli/src/claude/utils/startHappyServer.ts +++ b/cli/src/claude/utils/startHappyServer.ts @@ -13,12 +13,21 @@ import { logger } from "@/ui/logger"; import { ApiSessionClient } from "@/api/apiSession"; import { randomUUID } from "node:crypto"; import { detectImageMimeType, registerGeneratedImage } from "@/modules/common/generatedImages"; +import { resolveSkill } from "@/modules/common/skills"; type StartHappyServerOptions = { emitTitleSummary?: boolean; + skillLookup?: { + workingDirectory: string; + flavor: string; + }; }; -function createHapiMcpServer(client: ApiSessionClient, emitTitleSummary: boolean): McpServer { +function createHapiMcpServer( + client: ApiSessionClient, + emitTitleSummary: boolean, + skillLookup: StartHappyServerOptions['skillLookup'] +): McpServer { const handler = async (title: string) => { logger.debug('[hapiMCP] Changing title to:', title); try { @@ -50,6 +59,10 @@ function createHapiMcpServer(client: ApiSessionClient, emitTitleSummary: boolean title: z.string().optional().describe('Optional display title or filename for the image'), }); + const skillLookupInputSchema: z.ZodTypeAny = z.object({ + name: z.string().trim().min(1).max(128).describe('Exact skill name shown by HAPI skill autocomplete'), + }); + mcp.registerTool('change_title', { description: 'Change the title of the current chat session', title: 'Change Chat Title', @@ -145,6 +158,50 @@ function createHapiMcpServer(client: ApiSessionClient, emitTitleSummary: boolean } }); + if (skillLookup) { + mcp.registerTool('skill_lookup', { + description: 'Load a HAPI skill by exact name. When a user message starts with $name, call this tool with that name before acting.', + title: 'Look Up Skill', + inputSchema: skillLookupInputSchema, + }, async (args: { name: string }) => { + logger.debug('[hapiMCP] Looking up skill:', args.name); + try { + const skill = await resolveSkill(args.name, skillLookup.workingDirectory, { + flavor: skillLookup.flavor + }); + if (!skill) { + throw new Error(`Skill not found: ${args.name}`); + } + + const header = [ + `Skill: ${skill.name}`, + ...(skill.description ? [`Description: ${skill.description}`] : []) + ].join('\n'); + return { + content: [ + { + type: 'text' as const, + text: `${header}\n\n${skill.body}`, + }, + ], + isError: false, + }; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + logger.debug('[hapiMCP] Failed to look up skill:', message); + return { + content: [ + { + type: 'text' as const, + text: `Failed to look up skill: ${message}`, + }, + ], + isError: true, + }; + } + }); + } + return mcp; } @@ -165,7 +222,7 @@ export async function startHappyServer(client: ApiSessionClient, options: StartH const mcps = new Map(); const createMcpTransport = () => { - const mcp = createHapiMcpServer(client, emitTitleSummary); + const mcp = createHapiMcpServer(client, emitTitleSummary, options.skillLookup); const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), onsessioninitialized: (sessionId) => { @@ -219,9 +276,14 @@ export async function startHappyServer(client: ApiSessionClient, options: StartH hapiMcpUrl: mcpUrl, })); + const toolNames = ['change_title', 'display_image']; + if (options.skillLookup) { + toolNames.push('skill_lookup'); + } + return { url: mcpUrl, - toolNames: ['change_title', 'display_image'], + toolNames, stop: () => { logger.debug('[hapiMCP] Stopping server'); for (const mcp of mcps.values()) { diff --git a/cli/src/codex/happyMcpStdioBridge.test.ts b/cli/src/codex/happyMcpStdioBridge.test.ts new file mode 100644 index 00000000..5eed9974 --- /dev/null +++ b/cli/src/codex/happyMcpStdioBridge.test.ts @@ -0,0 +1,87 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +type ToolHandler = (args: Record) => Promise + +const harness = vi.hoisted(() => ({ + tools: new Map(), + callTool: vi.fn(async (_request: unknown) => ({ + content: [{ type: 'text', text: 'forwarded' }], + isError: false + })) +})) + +vi.mock('@modelcontextprotocol/sdk/server/mcp.js', () => ({ + McpServer: class { + registerTool(name: string, _config: unknown, handler: ToolHandler): void { + harness.tools.set(name, handler) + } + + async connect(): Promise {} + } +})) + +vi.mock('@modelcontextprotocol/sdk/server/stdio.js', () => ({ + StdioServerTransport: class {} +})) + +vi.mock('@modelcontextprotocol/sdk/client/index.js', () => ({ + Client: class { + async connect(): Promise {} + + async callTool(request: unknown): Promise { + return harness.callTool(request) + } + } +})) + +vi.mock('@modelcontextprotocol/sdk/client/streamableHttp.js', () => ({ + StreamableHTTPClientTransport: class { + constructor(_url: URL) {} + } +})) + +import { runHappyMcpStdioBridge } from './happyMcpStdioBridge' + +describe('runHappyMcpStdioBridge tool forwarding', () => { + beforeEach(() => { + harness.tools.clear() + harness.callTool.mockClear() + }) + + it('registers and forwards skill_lookup when the HTTP server enables it', async () => { + await runHappyMcpStdioBridge([ + '--url', + 'http://127.0.0.1:43006', + '--tools', + 'change_title,display_image,skill_lookup' + ]) + + expect([...harness.tools.keys()]).toEqual([ + 'change_title', + 'display_image', + 'skill_lookup' + ]) + + const handler = harness.tools.get('skill_lookup') + expect(handler).toBeDefined() + await expect(handler?.({ name: 'review' })).resolves.toEqual({ + content: [{ type: 'text', text: 'forwarded' }], + isError: false + }) + expect(harness.callTool).toHaveBeenCalledWith({ + name: 'skill_lookup', + arguments: { name: 'review' } + }) + }) + + it('keeps skill_lookup hidden when the upstream HTTP server does not enable it', async () => { + await runHappyMcpStdioBridge([ + '--url', + 'http://127.0.0.1:43006', + '--tools', + 'change_title,display_image' + ]) + + expect([...harness.tools.keys()]).toEqual(['change_title', 'display_image']) + }) +}) diff --git a/cli/src/codex/happyMcpStdioBridge.ts b/cli/src/codex/happyMcpStdioBridge.ts index 7c30617c..a2714f0a 100644 --- a/cli/src/codex/happyMcpStdioBridge.ts +++ b/cli/src/codex/happyMcpStdioBridge.ts @@ -17,22 +17,28 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import { z } from 'zod'; -function parseArgs(argv: string[]): { url: string | null } { +const DEFAULT_TOOL_NAMES = ['change_title', 'display_image']; + +function parseArgs(argv: string[]): { url: string | null; toolNames: Set } { let url: string | null = null; + let toolNames = new Set(DEFAULT_TOOL_NAMES); for (let i = 0; i < argv.length; i++) { const a = argv[i]; if (a === '--url' && i + 1 < argv.length) { url = argv[i + 1]; i++; + } else if (a === '--tools' && i + 1 < argv.length) { + toolNames = new Set(argv[i + 1].split(',').map((name) => name.trim()).filter(Boolean)); + i++; } } - return { url }; + return { url, toolNames }; } export async function runHappyMcpStdioBridge(argv: string[]): Promise { try { // Resolve target HTTP MCP URL - const { url: urlFromArgs } = parseArgs(argv); + const { url: urlFromArgs, toolNames } = parseArgs(argv); const baseUrl = urlFromArgs || process.env.HAPI_HTTP_MCP_URL || ''; if (!baseUrl) { @@ -69,29 +75,31 @@ export async function runHappyMcpStdioBridge(argv: string[]): Promise { title: z.string().describe('The new title for the chat session'), }); - server.registerTool( - 'change_title', - { - description: 'Change the title of the current chat session', - title: 'Change Chat Title', - inputSchema: changeTitleInputSchema, - }, - async (args: Record) => { - try { - const client = await ensureHttpClient(); - const response = await client.callTool({ name: 'change_title', arguments: args }); - // Pass-through response from HTTP server - return response as any; - } catch (error) { - return { - content: [ - { type: 'text' as const, text: `Failed to change chat title: ${error instanceof Error ? error.message : String(error)}` }, - ], - isError: true, - }; + if (toolNames.has('change_title')) { + server.registerTool( + 'change_title', + { + description: 'Change the title of the current chat session', + title: 'Change Chat Title', + inputSchema: changeTitleInputSchema, + }, + async (args: Record) => { + try { + const client = await ensureHttpClient(); + const response = await client.callTool({ name: 'change_title', arguments: args }); + // Pass-through response from HTTP server + return response as any; + } catch (error) { + return { + content: [ + { type: 'text' as const, text: `Failed to change chat title: ${error instanceof Error ? error.message : String(error)}` }, + ], + isError: true, + }; + } } - } - ); + ); + } @@ -100,28 +108,59 @@ export async function runHappyMcpStdioBridge(argv: string[]): Promise { title: z.string().optional().describe('Optional display title or filename for the image'), }); - server.registerTool( - 'display_image', - { - description: 'Display a local image file inline in the current HAPI chat session', - title: 'Display Image', - inputSchema: displayImageInputSchema, - }, - async (args: Record) => { - try { - const client = await ensureHttpClient(); - const response = await client.callTool({ name: 'display_image', arguments: args }); - return response as any; - } catch (error) { - return { - content: [ - { type: 'text' as const, text: `Failed to display image: ${error instanceof Error ? error.message : String(error)}` }, - ], - isError: true, - }; + if (toolNames.has('display_image')) { + server.registerTool( + 'display_image', + { + description: 'Display a local image file inline in the current HAPI chat session', + title: 'Display Image', + inputSchema: displayImageInputSchema, + }, + async (args: Record) => { + try { + const client = await ensureHttpClient(); + const response = await client.callTool({ name: 'display_image', arguments: args }); + return response as any; + } catch (error) { + return { + content: [ + { type: 'text' as const, text: `Failed to display image: ${error instanceof Error ? error.message : String(error)}` }, + ], + isError: true, + }; + } } - } - ); + ); + } + + const skillLookupInputSchema: z.ZodTypeAny = z.object({ + name: z.string().trim().min(1).max(128).describe('Exact skill name shown by HAPI skill autocomplete'), + }); + + if (toolNames.has('skill_lookup')) { + server.registerTool( + 'skill_lookup', + { + description: 'Load a HAPI skill by exact name. When a user message starts with $name, call this tool with that name before acting.', + title: 'Look Up Skill', + inputSchema: skillLookupInputSchema, + }, + async (args: Record) => { + try { + const client = await ensureHttpClient(); + const response = await client.callTool({ name: 'skill_lookup', arguments: args }); + return response as any; + } catch (error) { + return { + content: [ + { type: 'text' as const, text: `Failed to look up skill: ${error instanceof Error ? error.message : String(error)}` }, + ], + isError: true, + }; + } + } + ); + } // Start STDIO transport const stdio = new StdioServerTransport(); diff --git a/cli/src/codex/utils/buildHapiMcpBridge.test.ts b/cli/src/codex/utils/buildHapiMcpBridge.test.ts new file mode 100644 index 00000000..57ea9bf6 --- /dev/null +++ b/cli/src/codex/utils/buildHapiMcpBridge.test.ts @@ -0,0 +1,72 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { ApiSessionClient } from '@/api/apiSession' + +const harness = vi.hoisted(() => ({ + startOptions: null as unknown, + cliArgs: [] as string[] +})) + +vi.mock('@/claude/utils/startHappyServer', () => ({ + startHappyServer: vi.fn(async (_client: unknown, options: { skillLookup?: unknown }) => { + harness.startOptions = options + return { + url: 'http://127.0.0.1:43006/', + toolNames: options.skillLookup + ? ['change_title', 'display_image', 'skill_lookup'] + : ['change_title', 'display_image'], + stop: vi.fn() + } + }) +})) + +vi.mock('@/utils/spawnHappyCLI', () => ({ + getHappyCliCommand: vi.fn((args: string[]) => { + harness.cliArgs = args + return { command: 'hapi', args } + }) +})) + +import { buildHapiMcpBridge } from './buildHapiMcpBridge' + +describe('buildHapiMcpBridge skill lookup config', () => { + const client = {} as ApiSessionClient + + beforeEach(() => { + harness.startOptions = null + harness.cliArgs = [] + }) + + it('forwards the enabled HTTP tool through STDIO and auto-approves it', async () => { + const skillLookup = { + workingDirectory: '/repo', + flavor: 'opencode' + } + + const bridge = await buildHapiMcpBridge(client, { skillLookup }) + + expect(harness.startOptions).toEqual({ + emitTitleSummary: undefined, + skillLookup + }) + expect(harness.cliArgs).toEqual([ + 'mcp', + '--url', + 'http://127.0.0.1:43006/', + '--tools', + 'change_title,display_image,skill_lookup' + ]) + expect(bridge.mcpServers.hapi.tools).toEqual({ + change_title: { approval_mode: 'approve' }, + skill_lookup: { approval_mode: 'approve' } + }) + }) + + it('does not expose skill_lookup for native-skill bridge callers', async () => { + const bridge = await buildHapiMcpBridge(client) + + expect(harness.cliArgs.at(-1)).toBe('change_title,display_image') + expect(bridge.mcpServers.hapi.tools).toEqual({ + change_title: { approval_mode: 'approve' } + }) + }) +}) diff --git a/cli/src/codex/utils/buildHapiMcpBridge.ts b/cli/src/codex/utils/buildHapiMcpBridge.ts index 72ef85ea..52342e05 100644 --- a/cli/src/codex/utils/buildHapiMcpBridge.ts +++ b/cli/src/codex/utils/buildHapiMcpBridge.ts @@ -44,6 +44,10 @@ export interface HapiMcpBridge { export interface HapiMcpBridgeOptions { emitTitleSummary?: boolean; + skillLookup?: { + workingDirectory: string; + flavor: string; + }; } /** @@ -58,9 +62,26 @@ export async function buildHapiMcpBridge( options: HapiMcpBridgeOptions = {} ): Promise { const happyServer = await startHappyServer(client, { - emitTitleSummary: options.emitTitleSummary + emitTitleSummary: options.emitTitleSummary, + skillLookup: options.skillLookup }); - const bridgeCommand = getHappyCliCommand(['mcp', '--url', happyServer.url]); + const bridgeCommand = getHappyCliCommand([ + 'mcp', + '--url', + happyServer.url, + '--tools', + happyServer.toolNames.join(',') + ]); + const tools: Record = { + change_title: { + approval_mode: 'approve' + } + }; + if (options.skillLookup) { + tools.skill_lookup = { + approval_mode: 'approve' + }; + } return { server: { @@ -71,11 +92,7 @@ export async function buildHapiMcpBridge( hapi: { command: bridgeCommand.command, args: bridgeCommand.args, - tools: { - change_title: { - approval_mode: 'approve' - } - } + tools } } }; diff --git a/cli/src/cursor/cursorAcpRemoteLauncher.test.ts b/cli/src/cursor/cursorAcpRemoteLauncher.test.ts index 761e86c3..56a387ea 100644 --- a/cli/src/cursor/cursorAcpRemoteLauncher.test.ts +++ b/cli/src/cursor/cursorAcpRemoteLauncher.test.ts @@ -9,6 +9,7 @@ const harness = vi.hoisted(() => ({ loadSessionCalled: false, newSessionCalled: false, promptCalls: 0, + prompts: [] as unknown[][], backendArgs: null as { command: string; args?: string[] } | null, setConfigOptionCalls: [] as Array<{ sessionId: string; configId: string; value: string }>, deferSetConfigOption: null as Promise | null, @@ -89,8 +90,9 @@ vi.mock('./utils/cursorAcpBackend', () => ({ } return undefined; }), - prompt: vi.fn(async () => { + prompt: vi.fn(async (_sessionId: string, content: unknown[]) => { harness.promptCalls++; + harness.prompts.push(content); }), cancelPrompt: vi.fn(async () => {}), respondToPermission: vi.fn(async () => {}), @@ -182,6 +184,7 @@ describe('cursorAcpRemoteLauncher', () => { harness.loadSessionCalled = false; harness.newSessionCalled = false; harness.promptCalls = 0; + harness.prompts = []; harness.setConfigOptionCalls = []; harness.deferSetConfigOption = null; harness.releaseSetConfigOption = null; @@ -765,5 +768,8 @@ describe('cursorAcpRemoteLauncher', () => { await cursorAcpRemoteLauncher(session); expect(harness.promptCalls).toBe(2); + expect(JSON.stringify(harness.prompts[0])).toContain('$name'); + expect(JSON.stringify(harness.prompts[0])).toContain('skill_lookup'); + expect(JSON.stringify(harness.prompts[1])).not.toContain('skill_lookup'); }); }); diff --git a/cli/src/cursor/cursorAcpRemoteLauncher.ts b/cli/src/cursor/cursorAcpRemoteLauncher.ts index 0023dc97..3b20c6ca 100644 --- a/cli/src/cursor/cursorAcpRemoteLauncher.ts +++ b/cli/src/cursor/cursorAcpRemoteLauncher.ts @@ -30,6 +30,7 @@ import { cursorPassThroughStatusMessage, parseCursorSpecialCommand } from './cur import { buildCursorModelsSeedPayload, seedCursorModelsCache } from '@/modules/common/cursorModels'; import { readSharedCursorModelsCache } from '@/modules/common/cursorModelsSharedCache'; import type { AcpSdkBackend } from '@/agent/backends/acp'; +import { SKILL_LOOKUP_INSTRUCTION } from '@/modules/common/skillLookupInstruction'; class CursorAcpRemoteLauncher extends RemoteLauncherBase { private readonly session: CursorSession; @@ -47,6 +48,7 @@ class CursorAcpRemoteLauncher extends RemoteLauncherBase { private spawnedWithAutoReview = false; /** Avoid re-queueing `/auto-review` on every mid-session mode sync. */ private autoReviewSlashQueued = false; + private skillLookupInstructionSent = false; constructor(session: CursorSession) { super(process.env.DEBUG ? session.logPath : undefined); @@ -68,7 +70,9 @@ class CursorAcpRemoteLauncher extends RemoteLauncherBase { const session = this.session; const messageBuffer = this.messageBuffer; - const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client); + const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client, { + skillLookup: { workingDirectory: session.path, flavor: 'cursor' } + }); this.happyServer = happyServer; const autoReview = isCursorAutoReviewMode(session.getPermissionMode() as PermissionMode); @@ -239,9 +243,15 @@ class CursorAcpRemoteLauncher extends RemoteLauncherBase { } messageBuffer.addMessage(batch.message, 'user'); + let messageText = batch.message; + if (!this.skillLookupInstructionSent && !messageText.trimStart().startsWith('/')) { + messageText = `${SKILL_LOOKUP_INSTRUCTION}\n\n${messageText}`; + this.skillLookupInstructionSent = true; + } + const promptContent: PromptContent[] = [{ type: 'text', - text: batch.message + text: messageText }]; session.onThinkingChange(true); diff --git a/cli/src/grok/grokRemoteLauncher.test.ts b/cli/src/grok/grokRemoteLauncher.test.ts index 726d00fb..9fa8e979 100644 --- a/cli/src/grok/grokRemoteLauncher.test.ts +++ b/cli/src/grok/grokRemoteLauncher.test.ts @@ -159,7 +159,10 @@ describe('grokRemoteLauncher runtime config', () => { })) expect(JSON.stringify(harness.prompts[0])).toContain('/always-approve off') expect(JSON.stringify(harness.prompts[1])).toContain('hapi_change_title') + expect(JSON.stringify(harness.prompts[1])).toContain('$name') + expect(JSON.stringify(harness.prompts[1])).toContain('skill_lookup') expect(JSON.stringify(harness.prompts[2])).not.toContain('hapi_change_title') + expect(JSON.stringify(harness.prompts[2])).not.toContain('skill_lookup') expect(await rpcHandlers.get('listGrokModels')?.()).toMatchObject({ success: true, currentModelId: 'grok-a' }) expect(await rpcHandlers.get('listGrokReasoningEffortOptions')?.()).toMatchObject({ success: true, currentValue: 'low' }) }) diff --git a/cli/src/grok/grokRemoteLauncher.ts b/cli/src/grok/grokRemoteLauncher.ts index f1322676..6ac0de4d 100644 --- a/cli/src/grok/grokRemoteLauncher.ts +++ b/cli/src/grok/grokRemoteLauncher.ts @@ -67,7 +67,9 @@ class GrokRemoteLauncher extends RemoteLauncherBase { protected async runMainLoop(): Promise { const session = this.session - const { server, mcpServers } = await buildHapiMcpBridge(session.client) + const { server, mcpServers } = await buildHapiMcpBridge(session.client, { + skillLookup: { workingDirectory: session.path, flavor: 'grok' } + }) this.happyServer = server const backend = createGrokBackend({ diff --git a/cli/src/grok/utils/systemPrompt.ts b/cli/src/grok/utils/systemPrompt.ts index c4394141..8579d8a4 100644 --- a/cli/src/grok/utils/systemPrompt.ts +++ b/cli/src/grok/utils/systemPrompt.ts @@ -1,2 +1,4 @@ +import { SKILL_LOOKUP_INSTRUCTION } from '@/modules/common/skillLookupInstruction' + export const GROK_TITLE_INSTRUCTION = - 'Use the tool "hapi_change_title" once after the initial request is clear to set a concise session title. Do not rename for routine progress or substeps.' + `Use the tool "hapi_change_title" once after the initial request is clear to set a concise session title. Do not rename for routine progress or substeps.\n${SKILL_LOOKUP_INSTRUCTION}` diff --git a/cli/src/kimi/kimiRemoteLauncher.test.ts b/cli/src/kimi/kimiRemoteLauncher.test.ts new file mode 100644 index 00000000..0fb721c0 --- /dev/null +++ b/cli/src/kimi/kimiRemoteLauncher.test.ts @@ -0,0 +1,84 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { MessageQueue2 } from '@/utils/MessageQueue2' +import type { KimiMode } from './types' + +const harness = vi.hoisted(() => ({ + prompts: [] as unknown[][] +})) + +vi.mock('./utils/kimiBackend', () => ({ + createKimiBackend: vi.fn(() => ({ + initialize: vi.fn(async () => {}), + newSession: vi.fn(async () => 'kimi-session-1'), + loadSession: vi.fn(async () => 'kimi-session-1'), + setModel: vi.fn(async () => {}), + prompt: vi.fn(async (_sessionId: string, content: unknown[]) => { + harness.prompts.push(content) + }), + cancelPrompt: vi.fn(async () => {}), + respondToPermission: vi.fn(async () => {}), + onStderrError: vi.fn(), + onPermissionRequest: vi.fn(), + disconnect: vi.fn(async () => {}) + })) +})) + +vi.mock('@/codex/utils/buildHapiMcpBridge', () => ({ + buildHapiMcpBridge: async () => ({ + server: { stop: () => {} }, + mcpServers: {} + }) +})) + +vi.mock('./utils/permissionHandler', () => ({ + KimiPermissionHandler: class { + async cancelAll(): Promise {} + } +})) + +vi.mock('@/ui/ink/KimiDisplay', () => ({ KimiDisplay: () => null })) +vi.mock('@/ui/logger', () => ({ + logger: { debug: vi.fn(), warn: vi.fn(), info: vi.fn() } +})) + +import { kimiRemoteLauncher } from './kimiRemoteLauncher' + +function createSession() { + const queue = new MessageQueue2((mode) => JSON.stringify(mode)) + queue.pushIsolateAndClear('first', { permissionMode: 'default', model: 'kimi-k2' }) + queue.push('second', { permissionMode: 'default', model: 'kimi-k2' }) + queue.close() + + const session = { + path: '/tmp/kimi-test', + logPath: '/tmp/kimi-test/test.log', + client: { + rpcHandlerManager: { registerHandler: vi.fn() }, + sendAgentMessage: vi.fn(), + sendSessionEvent: vi.fn() + }, + queue, + sessionId: null as string | null, + getPermissionMode: () => 'default' as const, + onSessionFound(id: string) { session.sessionId = id }, + onThinkingChange: vi.fn(), + sendAgentMessage: vi.fn(), + sendSessionEvent: vi.fn() + } + return session +} + +describe('kimiRemoteLauncher skill lookup instruction', () => { + afterEach(() => { + harness.prompts = [] + }) + + it('injects the instruction only on the first prompt', async () => { + await kimiRemoteLauncher(createSession() as never, { model: 'kimi-k2' }) + + expect(harness.prompts).toHaveLength(2) + expect(JSON.stringify(harness.prompts[0])).toContain('$name') + expect(JSON.stringify(harness.prompts[0])).toContain('skill_lookup') + expect(JSON.stringify(harness.prompts[1])).not.toContain('skill_lookup') + }) +}) diff --git a/cli/src/kimi/kimiRemoteLauncher.ts b/cli/src/kimi/kimiRemoteLauncher.ts index 21fb438f..dc4c79bc 100644 --- a/cli/src/kimi/kimiRemoteLauncher.ts +++ b/cli/src/kimi/kimiRemoteLauncher.ts @@ -10,6 +10,7 @@ import type { PermissionMode } from './types'; import { createKimiBackend } from './utils/kimiBackend'; import { KimiPermissionHandler } from './utils/permissionHandler'; import { resolveKimiRuntimeConfig } from './utils/config'; +import { SKILL_LOOKUP_INSTRUCTION } from '@/modules/common/skillLookupInstruction'; class KimiRemoteLauncher extends RemoteLauncherBase { private readonly session: KimiSession; @@ -23,6 +24,7 @@ class KimiRemoteLauncher extends RemoteLauncherBase { private currentBackendModel: string | null = null; private setModelSupported: boolean | undefined = undefined; private lastDisplayedToolCall = new Map(); + private skillLookupInstructionSent = false; constructor(session: KimiSession, opts: { model?: string }) { super(process.env.DEBUG ? session.logPath : undefined); @@ -45,7 +47,9 @@ class KimiRemoteLauncher extends RemoteLauncherBase { const session = this.session; const messageBuffer = this.messageBuffer; - const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client); + const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client, { + skillLookup: { workingDirectory: session.path, flavor: 'kimi' } + }); this.happyServer = happyServer; const runtimeConfig = resolveKimiRuntimeConfig({ model: this.model }); @@ -156,9 +160,15 @@ class KimiRemoteLauncher extends RemoteLauncherBase { this.applyDisplayMode(batch.mode.permissionMode, batch.mode.model); messageBuffer.addMessage(batch.message, 'user'); + let messageText = batch.message; + if (!this.skillLookupInstructionSent && !messageText.trimStart().startsWith('/')) { + messageText = `${SKILL_LOOKUP_INSTRUCTION}\n\n${messageText}`; + this.skillLookupInstructionSent = true; + } + const promptContent: PromptContent[] = [{ type: 'text', - text: batch.message + text: messageText }]; session.onThinkingChange(true); diff --git a/cli/src/modules/common/permission/BasePermissionHandler.test.ts b/cli/src/modules/common/permission/BasePermissionHandler.test.ts new file mode 100644 index 00000000..6d18a194 --- /dev/null +++ b/cli/src/modules/common/permission/BasePermissionHandler.test.ts @@ -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() + }) +}) diff --git a/cli/src/modules/common/permission/BasePermissionHandler.ts b/cli/src/modules/common/permission/BasePermissionHandler.ts index f70908f5..2c12908d 100644 --- a/cli/src/modules/common/permission/BasePermissionHandler.ts +++ b/cli/src/modules/common/permission/BasePermissionHandler.ts @@ -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; } diff --git a/cli/src/modules/common/skillLookupInstruction.ts b/cli/src/modules/common/skillLookupInstruction.ts new file mode 100644 index 00000000..86736a1a --- /dev/null +++ b/cli/src/modules/common/skillLookupInstruction.ts @@ -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.' diff --git a/cli/src/modules/common/skills.test.ts b/cli/src/modules/common/skills.test.ts index 08813e2e..2efecd06 100644 --- a/cli/src/modules/common/skills.test.ts +++ b/cli/src/modules/common/skills.test.ts @@ -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 { 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') + }) }) diff --git a/cli/src/modules/common/skills.ts b/cli/src/modules/common/skills.ts index a9db9e44..954e3c0b 100644 --- a/cli/src/modules/common/skills.ts +++ b/cli/src/modules/common/skills.ts @@ -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; }; +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 { - const skills = await Promise.all(skillDirs.map(async (dir): Promise => { - 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 { + const skills = await Promise.all(skillDirs.map(async (dir): Promise => { + 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 { .map((installPath) => join(installPath, 'skills')); } -export async function listSkills(workingDirectory?: string, options: { flavor?: string } = {}): Promise { +async function discoverSkills(workingDirectory?: string, options: { flavor?: string } = {}): Promise { 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(); + const dedupedSkills = new Map(); 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 { + 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 { + 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 + }; +} diff --git a/cli/src/opencode/opencodeLocalLauncher.ts b/cli/src/opencode/opencodeLocalLauncher.ts index d8609dba..544f9c21 100644 --- a/cli/src/opencode/opencodeLocalLauncher.ts +++ b/cli/src/opencode/opencodeLocalLauncher.ts @@ -266,7 +266,9 @@ export async function opencodeLocalLauncher( let happyServer: { url: string; stop: () => void } | null = null; let opencodeConfigPath: string | null = null; try { - const bridge = await buildHapiMcpBridge(session.client); + const bridge = await buildHapiMcpBridge(session.client, { + skillLookup: { workingDirectory: session.path, flavor: 'opencode' } + }); happyServer = bridge.server; logger.debug(`[opencode-local]: Started hapi MCP server at ${happyServer.url}`); diff --git a/cli/src/opencode/opencodeRemoteLauncher.test.ts b/cli/src/opencode/opencodeRemoteLauncher.test.ts index 173fcbf7..4a369c4e 100644 --- a/cli/src/opencode/opencodeRemoteLauncher.test.ts +++ b/cli/src/opencode/opencodeRemoteLauncher.test.ts @@ -178,6 +178,19 @@ describe('opencodeRemoteLauncher inline model switch', () => { harness.thoughtLevelOption = null; }); + it('injects the skill lookup instruction only on the first prompt', async () => { + const { session } = createSessionStub([ + { message: 'first', mode: createMode() }, + { message: 'second', mode: createMode() } + ]); + + await opencodeRemoteLauncher(session as never); + + expect(JSON.stringify(harness.promptContents[0])).toContain('$name'); + expect(JSON.stringify(harness.promptContents[0])).toContain('skill_lookup'); + expect(JSON.stringify(harness.promptContents[1])).not.toContain('skill_lookup'); + }); + it('calls setModel with opencode flavor between turns when the queued model differs', async () => { const { session } = createSessionStub([ { message: 'first', mode: createMode('ollama/exaone:4.5-33b-q8') }, diff --git a/cli/src/opencode/opencodeRemoteLauncher.ts b/cli/src/opencode/opencodeRemoteLauncher.ts index b911fbca..9ccf6a41 100644 --- a/cli/src/opencode/opencodeRemoteLauncher.ts +++ b/cli/src/opencode/opencodeRemoteLauncher.ts @@ -55,7 +55,9 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { const session = this.session; const messageBuffer = this.messageBuffer; - const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client); + const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client, { + skillLookup: { workingDirectory: session.path, flavor: 'opencode' } + }); this.happyServer = happyServer; const backend = createOpencodeBackend({ diff --git a/cli/src/opencode/utils/systemPrompt.test.ts b/cli/src/opencode/utils/systemPrompt.test.ts new file mode 100644 index 00000000..b22d3f9e --- /dev/null +++ b/cli/src/opencode/utils/systemPrompt.test.ts @@ -0,0 +1,30 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { mkdtemp, readFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { ensureOpencodeConfig } from './opencodeConfig' +import { TITLE_INSTRUCTION } from './systemPrompt' + +describe('OpenCode local HAPI instructions', () => { + let configDirectory: string | null = null + + afterEach(async () => { + if (configDirectory) { + await rm(configDirectory, { recursive: true, force: true }) + configDirectory = null + } + }) + + it('writes the skill lookup instruction into the configured system prompt', async () => { + configDirectory = await mkdtemp(join(tmpdir(), 'hapi-opencode-prompt-')) + const { instructionsPath } = ensureOpencodeConfig( + configDirectory, + { command: 'hapi', args: ['mcp'] }, + TITLE_INSTRUCTION + ) + + const instructions = await readFile(instructionsPath, 'utf8') + expect(instructions).toContain('$name') + expect(instructions).toContain('skill_lookup') + }) +}) diff --git a/cli/src/opencode/utils/systemPrompt.ts b/cli/src/opencode/utils/systemPrompt.ts index e9687596..1061279b 100644 --- a/cli/src/opencode/utils/systemPrompt.ts +++ b/cli/src/opencode/utils/systemPrompt.ts @@ -6,6 +6,7 @@ */ import { trimIdent } from '@/utils/trimIdent'; +import { SKILL_LOOKUP_INSTRUCTION } from '@/modules/common/skillLookupInstruction'; /** * Title instruction for OpenCode to call the hapi MCP tool. @@ -13,6 +14,7 @@ import { trimIdent } from '@/utils/trimIdent'; export const TITLE_INSTRUCTION = trimIdent(` Use the title tool sparingly. For a new chat, call the tool "hapi_change_title" once after the user's initial request is clear, and set a concise task title. Do not rename the chat for routine progress, substeps, implementation details, or a slightly better wording. Rename only when the user's primary objective changes substantially and the existing title would be misleading. When you create or find a local image file that the user should see, call the tool "hapi_display_image" with the image path so HAPI can show it inline. + ${SKILL_LOOKUP_INSTRUCTION} `); /**