From 1b9341b4741a13649ba45243353b5693ea7fb96a Mon Sep 17 00:00:00 2001 From: Eshwar Sundar Date: Sat, 5 Sep 2026 11:19:00 +0530 Subject: [PATCH] feat(mcp): read discovered skill files remotely --- shared/glean/mcp/skills/glean_run/SKILL.md | 18 +- shared/glean/mcp/src/index.ts | 130 +++++-- shared/glean/mcp/src/policy/enforce.ts | 19 +- shared/glean/mcp/src/skill-files.ts | 242 +++++++++++++ shared/glean/mcp/src/skill-writer.ts | 160 --------- shared/glean/mcp/src/tools/find-skills.ts | 37 +- .../glean/mcp/src/tools/read-skill-files.ts | 45 +++ shared/glean/mcp/src/tools/run-tool.ts | 50 +-- shared/glean/mcp/src/types.ts | 6 +- shared/glean/mcp/start.mjs | 28 -- shared/glean/mcp/tests/find-skills.test.ts | 40 +-- shared/glean/mcp/tests/policy-enforce.test.ts | 34 +- .../glean/mcp/tests/read-skill-files.test.ts | 63 ++++ shared/glean/mcp/tests/run-tool.test.ts | 135 ++++--- shared/glean/mcp/tests/setup-output.test.ts | 6 +- shared/glean/mcp/tests/skill-files.test.ts | 76 ++++ shared/glean/mcp/tests/skill-writer.test.ts | 336 ------------------ shared/glean/mcp/tests/version.test.ts | 1 - 18 files changed, 690 insertions(+), 736 deletions(-) create mode 100644 shared/glean/mcp/src/skill-files.ts delete mode 100644 shared/glean/mcp/src/skill-writer.ts create mode 100644 shared/glean/mcp/src/tools/read-skill-files.ts create mode 100644 shared/glean/mcp/tests/read-skill-files.test.ts create mode 100644 shared/glean/mcp/tests/skill-files.test.ts delete mode 100644 shared/glean/mcp/tests/skill-writer.test.ts diff --git a/shared/glean/mcp/skills/glean_run/SKILL.md b/shared/glean/mcp/skills/glean_run/SKILL.md index 26bacc2..60d3eb9 100644 --- a/shared/glean/mcp/skills/glean_run/SKILL.md +++ b/shared/glean/mcp/skills/glean_run/SKILL.md @@ -2,8 +2,6 @@ name: glean_run description: Discover and run Glean skills for enterprise app tasks argument-hint: -allowed-tools: - - Read(path="//**/glean-skills-cache/**") --- # Glean Run @@ -64,7 +62,7 @@ find_skills_and_tools({ }) ``` -The response is an XML index of discovered skills with file paths. +The response is an XML index of discovered skills and their available file paths. You can call `find_skills_and_tools` multiple times — e.g. to discover skills for individual sub-tasks as you work through a broad request. @@ -72,16 +70,18 @@ individual sub-tasks as you work through a broad request. ## Step 2: Read Skill Instructions Browse the returned skills and select the one most relevant to the user's -request. Read its `SKILL.md` file for detailed instructions. Skills typically -contain guidance on how to use their tools, but the tools can also be called -as independent units. +request. Call `read_skill_files` with the selected skill name and `SKILL.md` +to retrieve its detailed instructions. Skills typically contain guidance on +how to use their tools, but the tools can also be called as independent units. ## Step 3: Read Tool Schemas -Read each tool's JSON file (e.g. `tools/TOOL_NAME.json`) to get the exact -`server_id`, `name`, and `inputSchema` with parameter names and types. +Call `read_skill_files` for each tool's JSON file (e.g. +`tools/TOOL_NAME.json`) to get the exact `server_id`, `name`, and `inputSchema` +with parameter names and types. -**Never guess parameter names** - always read the tool JSON file first. +**Never guess parameter names** - always call `read_skill_files` for the tool +JSON before calling `run_tool`. ## Step 4: Execute Tools diff --git a/shared/glean/mcp/src/index.ts b/shared/glean/mcp/src/index.ts index 4d5255b..cdb8fb6 100644 --- a/shared/glean/mcp/src/index.ts +++ b/shared/glean/mcp/src/index.ts @@ -8,7 +8,6 @@ import { } from "@modelcontextprotocol/sdk/types.js"; import path from "node:path"; import fs from "node:fs"; -import { homedir, tmpdir } from "node:os"; import { AuthRequiredError, createRemoteClient, @@ -20,8 +19,9 @@ import { closeCallbackServer, } from "./auth-callback-server.js"; import { handleFindSkills } from "./tools/find-skills.js"; +import { handleReadSkillFiles } from "./tools/read-skill-files.js"; import { handleRunTool, runToolAnnotations } from "./tools/run-tool.js"; -import { evictStaleSkills } from "./skill-writer.js"; +import { SkillFileCache } from "./skill-files.js"; import { loadServerUrl, saveServerUrl, @@ -127,13 +127,6 @@ function logLine(label: string, detail?: Record): void { console.error(line.trimEnd()); } -function resolveSkillsBaseDir(): string { - if (process.env.SKILLS_BASE_DIR) { - return process.env.SKILLS_BASE_DIR; - } - return path.join(tmpdir(), "glean-skills-cache"); -} - const server = new Server( { name: "glean", version: pluginVersionString() }, { capabilities: { tools: { listChanged: true } } }, @@ -156,6 +149,11 @@ let oauthProvider: GleanOAuthClientProvider | undefined; // successful tool fetch. let cachedRemoteTools: Tool[] = loadRemoteTools(resolveServerUrl() ?? ""); +// Skill contents are owned by the remote server. The plugin retains only the +// tool metadata needed by local HITL and file_args handling, plus an in-memory +// compatibility cache for older servers that still return full JSON skills. +const skillFiles = new SkillFileCache(); + function getOAuthProvider(): GleanOAuthClientProvider { if (!oauthProvider) { oauthProvider = new GleanOAuthClientProvider(); @@ -193,18 +191,16 @@ const FIND_SKILLS_TOOL: Tool = { "timing, reasons, constraints) — and pass each as a separate entry in the " + "'queries' array. For example, for \"Send an email to X for tomorrow's demo " + "meeting as leadership will be visiting\", the single query is \"send an email\". " + - "Discovered skills are written to local files and an XML skill " + - "index with usage instructions is returned. " + + "An XML skill index with the available file paths is returned. " + "If a returned skill lists no tools and its playbook does not let you " + "complete the task, first check whether tools already in scope can do it — " + "tools from other skills in this response, tools from earlier find_skills_and_tools " + "calls, or tools you can already call directly. If none fit, call find_skills_and_tools " + "again with reworded or additional queries. " + - "If a previously-cached skill file referenced from memory or instructions " + - "is missing on disk, call find_skills_and_tools again to re-fetch it before failing. " + "To use a returned skill: (1) pick the most relevant from the returned " + - "skills; (2) read its SKILL.md for instructions; (3) read each tool's JSON " + - "file (tools/TOOL_NAME.json) for the exact server_id, name, and inputSchema " + + "skills; (2) call read_skill_files for SKILL.md and read it for instructions; " + + "(3) call read_skill_files for each tool JSON file " + + "(tools/TOOL_NAME.json) to get the exact server_id, name, and inputSchema " + "(exact parameter names and types); (4) call run_tool with the server_id, " + "tool_name (from the name field), and arguments matching the inputSchema. " + "Never guess parameter names — read the tool JSON file first.", @@ -225,11 +221,38 @@ const FIND_SKILLS_TOOL: Tool = { }, }; +const READ_SKILL_FILES_TOOL: Tool = { + name: "read_skill_files", + annotations: { readOnlyHint: true }, + description: + "Read one or more files from a Glean skill returned by a previous " + + "find_skills_and_tools call. Use this to read SKILL.md and tools/*.json " + + "before using run_tool. Pass the exact skill name and file paths from the " + + "find_skills_and_tools response.", + inputSchema: { + type: "object" as const, + properties: { + skill_name: { + type: "string", + description: "The skill name from a previous find_skills_and_tools response.", + }, + file_paths: { + type: "array", + items: { type: "string" }, + minItems: 1, + description: + "Paths within the skill to read, such as SKILL.md or tools/TOOL_NAME.json.", + }, + }, + required: ["skill_name", "file_paths"], + }, +}; + const RUN_TOOL_TOOL: Tool = { name: "run_tool", description: "Execute a tool on a downstream MCP server. Before calling this tool, " + - "you MUST read the tool's JSON file from the find_skills_and_tools output to get " + + "you MUST call read_skill_files for the tool's JSON file to get " + "the exact server_id, tool_name, and input_schema. Pass arguments that match " + "the input_schema exactly — do not guess parameter names.", inputSchema: { @@ -322,6 +345,7 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { decision, setupTool: SETUP_TOOL, findSkillsTool: FIND_SKILLS_TOOL, + readSkillFilesTool: READ_SKILL_FILES_TOOL, runTool, promoted: dynamic, }); @@ -663,8 +687,6 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { const sessionId = resolveSessionId(); - const skillsBaseDir = resolveSkillsBaseDir(); - let remoteClient; try { remoteClient = await createRemoteClient( @@ -691,11 +713,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { }; } try { - const text = await handleFindSkills( - remoteClient, - skillsBaseDir, - args, - ); + const text = await handleFindSkills(remoteClient, skillFiles, args); return { content: [{ type: "text", text }] }; } catch (err) { const msg = err instanceof Error ? err.message : String(err); @@ -709,6 +727,58 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { } } + case "read_skill_files": { + const serverUrl = resolveServerUrl(); + if (!serverUrl) { + return { + content: [{ type: "text", text: SETUP_NEEDED_ERROR }], + isError: true, + }; + } + + if (!getOAuthProvider().tokens()) { + return { + content: [{ type: "text", text: AUTH_REDIRECT_TO_SETUP_TEXT }], + }; + } + + let remoteClient; + try { + remoteClient = await createRemoteClient( + serverUrl, + getRemoteClientOpts(), + resolveSessionId(), + ); + } catch (err) { + if (err instanceof AuthRequiredError) { + return { + content: [{ type: "text", text: AUTH_REDIRECT_TO_SETUP_TEXT }], + }; + } + const msg = err instanceof Error ? err.message : String(err); + logLine("connect.backend-error", { label: "read_skill_files", msg }); + return { + content: [ + { type: "text", text: `Failed to connect to Glean backend: ${msg}` }, + ], + isError: true, + }; + } + try { + const text = await handleReadSkillFiles(remoteClient, skillFiles, args); + return { content: [{ type: "text", text }] }; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`read_skill_files: execution failed: ${msg}`); + return { + content: [{ type: "text", text: `read_skill_files failed: ${msg}` }], + isError: true, + }; + } finally { + await remoteClient.close(); + } + } + case "run_tool": { const serverUrl = resolveServerUrl(); if (!serverUrl) { @@ -755,8 +825,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { }; } try { - const skillsBaseDir = resolveSkillsBaseDir(); - return await handleRunTool(remoteClient, server, skillsBaseDir, args, { + return await handleRunTool(remoteClient, server, skillFiles.metadata, args, { fileArgs: decision.features.fileArgs, }); } catch (err) { @@ -780,6 +849,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { clearServerUrl(); clearCredentials(); clearRemoteTools(); + skillFiles.clear(); oauthProvider = undefined; cachedRemoteTools = []; // Policy survives a user reset: only a new valid remote policy may @@ -862,6 +932,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { // instant); we just rehydrate from whatever cache exists for the // newly configured URL — empty for a first-time server. clearCredentials(); + skillFiles.clear(); oauthProvider = undefined; cachedRemoteTools = loadRemoteTools(normalized); setPolicyServerUrl(normalized); @@ -882,15 +953,6 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { }); async function main() { - // Run once per session at MCP server startup. - const ONE_WEEK_MS = 7 * 24 * 60 * 60 * 1000; - try { - await evictStaleSkills(resolveSkillsBaseDir(), ONE_WEEK_MS, logLine); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - logLine("evict-stale-skills.failed", { msg }); - } - // Observe the negotiated MCP protocol revision from the initialize response. const transport = protocolVersion.wrap(new StdioServerTransport()); await server.connect(transport); diff --git a/shared/glean/mcp/src/policy/enforce.ts b/shared/glean/mcp/src/policy/enforce.ts index 4e854e7..787faf5 100644 --- a/shared/glean/mcp/src/policy/enforce.ts +++ b/shared/glean/mcp/src/policy/enforce.ts @@ -5,10 +5,11 @@ import type { Decision } from "./types.js"; // because it is how a user restores the connection that would lift the deactivation. export const SETUP_TOOL_NAME = "setup"; -// Gated together by `metaTools`, and gated as a pair on purpose: a surface with -// find_skills_and_tools but no run_tool can discover work it cannot perform. +// Gated together by `metaTools`: discovery, file reads, and execution form one +// surface and must be withdrawn together. export const META_TOOL_NAMES: ReadonlySet = new Set([ "find_skills_and_tools", + "read_skill_files", "run_tool", ]); @@ -47,16 +48,25 @@ export function advertisedTools(input: { decision: Decision; setupTool: Tool; findSkillsTool: Tool; + readSkillFilesTool: Tool; runTool: Tool; promoted: Tool[]; }): Advertisement { - const { decision, setupTool, findSkillsTool, runTool, promoted } = input; + const { + decision, + setupTool, + findSkillsTool, + readSkillFilesTool, + runTool, + promoted, + } = input; if (decision.deactivated) { return { tools: [setupTool], withheld: [ findSkillsTool.name, + readSkillFilesTool.name, runTool.name, ...promoted.map((t) => t.name), ], @@ -69,10 +79,11 @@ export function advertisedTools(input: { if (decision.features.metaTools) { tools.push( findSkillsTool, + readSkillFilesTool, decision.features.fileArgs ? runTool : withoutFileArgs(runTool), ); } else { - withheld.push(findSkillsTool.name, runTool.name); + withheld.push(findSkillsTool.name, readSkillFilesTool.name, runTool.name); } tools.push(setupTool); diff --git a/shared/glean/mcp/src/skill-files.ts b/shared/glean/mcp/src/skill-files.ts new file mode 100644 index 0000000..d21c18c --- /dev/null +++ b/shared/glean/mcp/src/skill-files.ts @@ -0,0 +1,242 @@ +import yaml from "yaml"; +import type { SkillDirectoryMap, SkillsMap } from "./types.js"; + +export interface ToolInputSchema { + properties?: Record; +} + +export interface ToolMetadata { + requires_approval?: boolean; + name?: string; + tool_name?: string; + description?: string; + server_id?: string; + inputSchema?: ToolInputSchema; + input_schema?: ToolInputSchema; +} + +/** + * Runtime metadata learned from tools/*.json responses. This cache is process + * local and intentionally contains only tool metadata, not a disk-backed skill + * tree. Missing metadata is handled fail-closed by run_tool. + */ +export class ToolMetadataCache { + private readonly byServerAndName = new Map(); + private readonly byName = new Map(); + + clear(): void { + this.byServerAndName.clear(); + this.byName.clear(); + } + + set(metadata: ToolMetadata): void { + const toolName = metadata.name ?? metadata.tool_name; + if (typeof toolName !== "string" || toolName.length === 0) return; + + const normalized: ToolMetadata = { + ...metadata, + name: toolName, + inputSchema: metadata.inputSchema ?? metadata.input_schema, + }; + if (typeof normalized.server_id === "string" && normalized.server_id.length > 0) { + this.byServerAndName.set(this.key(normalized.server_id, toolName), normalized); + } + this.byName.set(toolName, normalized); + } + + get(serverId: string, toolName: string): ToolMetadata | null { + return ( + this.byServerAndName.get(this.key(serverId, toolName)) ?? + this.byName.get(toolName) ?? + null + ); + } + + ingestSkillFiles(skills: SkillsMap): void { + for (const fileMap of Object.values(skills)) { + this.ingestFileMap(fileMap); + } + } + + ingestReadResponse(response: string): void { + const filePattern = + /]*\bpath="([^"]+)"[^>]*><\/file>/g; + for (const match of response.matchAll(filePattern)) { + const filePath = decodeXml(match[1]); + if (!filePath.startsWith("tools/") || !filePath.endsWith(".json")) continue; + this.ingestToolJson(match[2].replaceAll("]]]]>", "]]>")); + } + } + + private ingestFileMap(fileMap: SkillDirectoryMap): void { + for (const [filePath, content] of Object.entries(fileMap)) { + if (!filePath.startsWith("tools/") || !filePath.endsWith(".json")) continue; + this.ingestToolJson(content); + } + } + + private ingestToolJson(content: string): void { + try { + const parsed = JSON.parse(content) as ToolMetadata; + if (parsed && typeof parsed === "object") this.set(parsed); + } catch { + // A malformed tool file must not weaken the fail-closed run_tool path. + } + } + + private key(serverId: string, toolName: string): string { + return `${serverId}\u0000${toolName}`; + } +} + +/** + * Compatibility-only in-memory store for the legacy find_skills response. It + * lets a newer client continue talking to an older server without writing the + * server's complete skill tree to local disk. + */ +export class LegacySkillCache { + private skills = new Map(); + + clear(): void { + this.skills.clear(); + } + + replace(skills: SkillsMap): void { + this.skills = new Map( + Object.entries(skills).map(([name, files]) => [name, { ...files }]), + ); + } + + read( + skillName: string, + filePaths: string[], + ): { files: Record; availableFiles: string[] } | null { + const fileMap = this.skills.get(skillName); + if (!fileMap) return null; + + const availableFiles = Object.keys(fileMap).sort(); + const files: Record = {}; + for (const filePath of filePaths) { + const content = fileMap[filePath]; + if (typeof content === "string") files[filePath] = content; + } + return { files, availableFiles }; + } +} + +export class SkillFileCache { + readonly metadata = new ToolMetadataCache(); + readonly legacy = new LegacySkillCache(); + + clear(): void { + this.metadata.clear(); + this.legacy.clear(); + } + + ingestLegacySkills(skills: SkillsMap): void { + this.legacy.replace(skills); + this.metadata.ingestSkillFiles(skills); + } +} + +export function parseLegacySkillsResponse(text: string): SkillsMap | null { + try { + const parsed = JSON.parse(text) as { skills?: unknown }; + if ( + !parsed || + typeof parsed.skills !== "object" || + parsed.skills === null || + Array.isArray(parsed.skills) + ) { + return null; + } + return parsed.skills as SkillsMap; + } catch { + return null; + } +} + +export function formatLegacySkillIndex(skills: SkillsMap): string { + const entries = Object.entries(skills).map(([skillName, files]) => { + const frontmatter = parseFrontmatter(files["SKILL.md"] ?? ""); + const description = frontmatter.description ?? ""; + const paths = Object.keys(files).sort(); + const fileLines = paths.map( + (filePath) => ` ${escapeXml(filePath)}`, + ); + return [ + ` `, + " ", + ...fileLines, + " ", + " ", + ].join("\n"); + }); + + return entries.length === 0 + ? "" + : ["", ...entries, ""].join("\n"); +} + +export function formatReadSkillFilesResponse( + filePaths: string[], + files: Record, + availableFiles: string[], +): string { + const lines = [""]; + for (const filePath of filePaths) { + const content = files[filePath]; + if (content !== undefined) { + lines.push( + ` `, + ); + } else { + lines.push(` `); + } + } + if (availableFiles.length > 0) { + lines.push(" "); + for (const filePath of availableFiles) { + lines.push(` ${escapeXml(filePath)}`); + } + lines.push(" "); + } + lines.push(""); + return lines.join("\n"); +} + +function parseFrontmatter(content: string): Record { + const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!match) return {}; + try { + const parsed = yaml.parse(match[1]); + if (!parsed || typeof parsed !== "object") return {}; + return Object.fromEntries( + Object.entries(parsed).filter((entry): entry is [string, string] => + typeof entry[1] === "string", + ), + ); + } catch { + return {}; + } +} + +function sanitizeCdata(content: string): string { + return content.replaceAll("]]>", "]]]]>"); +} + +function escapeXml(value: string): string { + return value + .replaceAll("&", "&") + .replaceAll("<", "<") + .replaceAll(">", ">") + .replaceAll('"', """); +} + +function decodeXml(value: string): string { + return value + .replaceAll(""", '"') + .replaceAll("<", "<") + .replaceAll(">", ">") + .replaceAll("&", "&"); +} diff --git a/shared/glean/mcp/src/skill-writer.ts b/shared/glean/mcp/src/skill-writer.ts deleted file mode 100644 index 42b0472..0000000 --- a/shared/glean/mcp/src/skill-writer.ts +++ /dev/null @@ -1,160 +0,0 @@ -import fs from "node:fs/promises"; -import path from "node:path"; -import yaml from "yaml"; -import type { SkillsMap, SkillIndex } from "./types.js"; - -function isInsideDir(filePath: string, dir: string): boolean { - const resolved = path.resolve(filePath); - return resolved.startsWith(path.resolve(dir) + path.sep); -} - -/** - * Parses YAML frontmatter from a SKILL.md string, returning key-value pairs - * for top-level scalar fields (name, description, etc.). - */ -function parseFrontmatter(content: string): Record { - // Extract the YAML block between --- delimiters, allowing CRLF line endings. - const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); - if (!match) { - return {}; - } - const result: Record = {}; - try { - const parsed = yaml.parse(match[1]); - if (parsed && typeof parsed === "object") { - for (const [key, value] of Object.entries(parsed)) { - if (typeof value === "string") { - result[key] = value; - } - } - } - } catch { - return {}; - } - return result; -} - -type LogFn = (label: string, detail?: Record) => void; - -/** - * Remove cached skill subdirectories whose mtime is older than `maxAgeMs`. - * `writeSkillsToDisk` rm-then-mkdir's a skill dir on every refetch, so dir - * mtime is a reliable last-refresh signal. Safe to evict aggressively — - * find_skills_and_tools re-fetches on demand if the agent references a skill whose - * files were removed. - */ -export async function evictStaleSkills( - baseDir: string, - maxAgeMs: number, - log?: LogFn, - now: number = Date.now(), -): Promise { - let entries; - try { - entries = await fs.readdir(baseDir, { withFileTypes: true }); - } catch { - return; - } - const cutoff = now - maxAgeMs; - await Promise.all( - entries.map(async (entry) => { - if (!entry.isDirectory()) return; - const skillDir = path.resolve(baseDir, entry.name); - if (!isInsideDir(skillDir, baseDir)) return; - try { - const stat = await fs.stat(skillDir); - if (stat.mtimeMs < cutoff) { - await fs.rm(skillDir, { recursive: true, force: true }); - log?.("evict-stale-skill", { skill: entry.name }); - } - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - log?.("evict-stale-skill.failed", { skill: entry.name, msg }); - } - }), - ); -} - -export async function writeSkillsToDisk( - skills: SkillsMap, - baseDir: string, -): Promise { - const index: SkillIndex[] = []; - - for (const [skillName, fileMap] of Object.entries(skills)) { - const skillDir = path.resolve(baseDir, skillName); - if (!isInsideDir(skillDir, baseDir)) { - continue; - } - - // Delete and re-create so re-fetched skills never serve stale files. - await fs.rm(skillDir, { recursive: true, force: true }); - await fs.mkdir(skillDir, { recursive: true }); - - const writtenFiles: string[] = []; - - for (const [filePath, content] of Object.entries(fileMap)) { - const fullPath = path.resolve(skillDir, filePath); - if (!isInsideDir(fullPath, skillDir)) { - continue; - } - await fs.mkdir(path.dirname(fullPath), { recursive: true }); - const text = - typeof content === "string" ? content : JSON.stringify(content); - await fs.writeFile(fullPath, text, "utf-8"); - writtenFiles.push(fullPath); - } - - const rawSkillMd = fileMap["SKILL.md"] ?? ""; - const skillMdContent = typeof rawSkillMd === "string" ? rawSkillMd : ""; - const frontmatter = parseFrontmatter(skillMdContent); - - index.push({ - name: frontmatter.name ?? skillName, - description: frontmatter.description ?? "", - skillDir, - files: writtenFiles, - }); - } - - return index; -} - -export function formatAvailableSkillsPrompt(index: SkillIndex[]): string { - if (index.length === 0) { - return ""; - } - - const skillEntries = index.map((entry) => { - // Match SKILL.md under either path separator: writeSkillsToDisk stores - // paths with the OS separator, so a hardcoded "/" drops the reference on - // Windows (where stored paths use "\"), leaving the model without a pointer - // to the skill's instructions. - const skillMd = entry.files.find((f) => /(?:^|[\\/])SKILL\.md$/.test(f)); - const fileLines = skillMd - ? `\n \n ` - : ""; - - return [ - ` `, - ` ${fileLines}`, - ` `, - ].join("\n"); - }); - - // Usage instructions live in the find_skills_and_tools tool description (advertised - // once at tools/list) rather than being re-emitted in every response. - return [ - "", - ...skillEntries, - "", - ].join("\n"); -} - -function escapeXml(s: string): string { - return s - .replace(/&/g, "&") - .replace(//g, ">") - .replace(/"/g, """); -} diff --git a/shared/glean/mcp/src/tools/find-skills.ts b/shared/glean/mcp/src/tools/find-skills.ts index 313b3bd..00a31cc 100644 --- a/shared/glean/mcp/src/tools/find-skills.ts +++ b/shared/glean/mcp/src/tools/find-skills.ts @@ -1,11 +1,14 @@ import type { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { callRemoteTool } from "../remote-client.js"; -import { writeSkillsToDisk, formatAvailableSkillsPrompt } from "../skill-writer.js"; -import type { SkillsMap } from "../types.js"; +import { + formatLegacySkillIndex, + parseLegacySkillsResponse, + type SkillFileCache, +} from "../skill-files.js"; export async function handleFindSkills( remoteClient: Client, - skillsBaseDir: string, + skillFiles: SkillFileCache, args: Record, ): Promise { const toolArgs: Record = {}; @@ -26,13 +29,25 @@ export async function handleFindSkills( throw new Error(textContent.text || "find_skills failed"); } - const parsed = JSON.parse(textContent.text) as { skills?: SkillsMap }; - if (!parsed.skills || typeof parsed.skills !== "object") { - console.error( - `find_skills: unexpected response shape, keys: ${Object.keys(parsed).join(", ")}`, - ); - return ""; + // A fresh discovery result is the authority for the current session. Drop + // metadata from previous discovery results so a changed approval policy can + // never reuse an old requires_approval value. + skillFiles.clear(); + + const text = textContent.text.trim(); + if (text.startsWith(""; } diff --git a/shared/glean/mcp/src/tools/read-skill-files.ts b/shared/glean/mcp/src/tools/read-skill-files.ts new file mode 100644 index 0000000..b280a55 --- /dev/null +++ b/shared/glean/mcp/src/tools/read-skill-files.ts @@ -0,0 +1,45 @@ +import type { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { callRemoteTool } from "../remote-client.js"; +import { + formatReadSkillFilesResponse, + type SkillFileCache, +} from "../skill-files.js"; + +export async function handleReadSkillFiles( + remoteClient: Client, + skillFiles: SkillFileCache, + args: Record, +): Promise { + const skillName = typeof args.skill_name === "string" ? args.skill_name : ""; + const filePaths = Array.isArray(args.file_paths) + ? args.file_paths.filter((path): path is string => typeof path === "string") + : []; + + // An older server has already returned the complete response to find_skills. + // Serve reads from the process-local compatibility cache instead of writing + // those files to disk or calling a tool the older server does not expose. + const legacy = skillFiles.legacy.read(skillName, filePaths); + if (legacy) { + return formatReadSkillFilesResponse( + filePaths, + legacy.files, + legacy.availableFiles, + ); + } + + const result = await callRemoteTool(remoteClient, "read_skill_files", { + skill_name: args.skill_name, + file_paths: args.file_paths, + }); + + const textContent = result.content.find((c) => c.type === "text"); + if (!textContent || textContent.type !== "text") { + return ""; + } + if (result.isError) { + throw new Error(textContent.text || "read_skill_files failed"); + } + + skillFiles.metadata.ingestReadResponse(textContent.text); + return textContent.text; +} diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 2436b6a..3aa437a 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -10,6 +10,7 @@ import { FILE_ARGS_DISABLED_TEXT } from "../policy/enforce.js"; import { buildCompactArgs, writeApprovalArgsFile } from "./approval-args.js"; import { resolveSessionId } from "../session-id.js"; import { hostSharedDataDir } from "../data-dir.js"; +import type { ToolMetadataCache, ToolInputSchema } from "../skill-files.js"; const DEFAULT_FILE_ARG_MAX_BYTES = 5 * 1024 * 1024; @@ -25,15 +26,6 @@ export class FileArgsError extends Error { } } -// A downstream tool parameter's JSON Schema, narrowed to the bits we use. -// `type` may be a single string or an array (e.g. ["object", "null"]). -interface ParamSchema { - type?: string | string[]; -} -interface ToolInputSchema { - properties?: Record; -} - // The set of JSON Schema types declared for a top-level parameter. file_args // keys always map to top-level argument names, so a direct properties lookup // is sufficient — no need to walk nested schemas. @@ -159,36 +151,6 @@ export async function resolveFileArgs( return merged; } -interface ToolMetadata { - requires_approval?: boolean; - name?: string; - description?: string; - server_id?: string; - inputSchema?: ToolInputSchema; -} - -async function findToolJson( - skillsBaseDir: string, - toolName: string, -): Promise { - try { - const skillDirs = await fs.readdir(skillsBaseDir, { withFileTypes: true }); - for (const dir of skillDirs) { - if (!dir.isDirectory()) continue; - const toolPath = path.join(skillsBaseDir, dir.name, "tools", `${toolName}.json`); - try { - const content = await fs.readFile(toolPath, "utf-8"); - return JSON.parse(content) as ToolMetadata; - } catch { - continue; - } - } - } catch { - // Skills dir doesn't exist or can't be read - } - return null; -} - // A stdio server's only client signal is clientInfo.name; Cursor reports // "cursor-vscode". Used to explain the known dropped-elicitation failure mode // when an approval request waits out its full timeout. @@ -315,7 +277,7 @@ export interface RunToolPolicy { export async function handleRunTool( remoteClient: Client, mcpServer: Server, - skillsBaseDir: string, + toolMetadata: ToolMetadataCache, args: Record, policy: RunToolPolicy, ): Promise { @@ -334,7 +296,7 @@ export async function handleRunTool( // Load the downstream tool's metadata once, up front: its inputSchema drives // file_args JSON-parsing (object/array params) and its requires_approval // drives the HITL gate. Both paths must see it regardless of ENABLE_HITL. - const toolMeta = await findToolJson(skillsBaseDir, toolName); + const toolMeta = toolMetadata.get(serverId, toolName); // Refuse before reading any model-supplied path. Disabled file_args must be // inert, not merely absent from the advertised schema. @@ -371,9 +333,9 @@ export async function handleRunTool( const hitlEnabled = process.env.ENABLE_HITL === "true"; // Fail CLOSED when the tool's approval requirement is unknown. The gate used - // to key on `toolMeta?.requires_approval`; a missing or unparseable tool JSON - // (evicted by evictStaleSkills after a week, called from memory without a - // fresh find_skills_and_tools, or corrupt) made that falsy, so the gate + // to key on `toolMeta?.requires_approval`; missing metadata (for example, + // when the model has not read the tool definition yet, or when it is corrupt) + // must never make the gate disappear, so the gate // was skipped and — with the native prompt already suppressed via // readOnlyHint — the tool executed with ZERO approval. Only skip the gate // when we can positively confirm the tool is read-only. diff --git a/shared/glean/mcp/src/types.ts b/shared/glean/mcp/src/types.ts index 9dfa92a..fa4ccfc 100644 --- a/shared/glean/mcp/src/types.ts +++ b/shared/glean/mcp/src/types.ts @@ -1,11 +1,11 @@ /** - * Wire format from find_skills: a flat map of slash-separated file paths to - * file contents (e.g. {"SKILL.md": "...", "tools/FOO.json": "..."}). + * Legacy wire format from find_skills: a flat map of slash-separated file + * paths to file contents. New servers return a lazy XML index instead. */ export type SkillDirectoryMap = Record; /** - * Wire format from find_skills: a map of skill names to their file maps. + * Legacy compatibility format: a map of skill names to their file maps. */ export type SkillsMap = Record; diff --git a/shared/glean/mcp/start.mjs b/shared/glean/mcp/start.mjs index 84e549b..86e0e3e 100644 --- a/shared/glean/mcp/start.mjs +++ b/shared/glean/mcp/start.mjs @@ -6,7 +6,6 @@ // env sanitation before launching the plugin proper. import os from "node:os"; import path from "node:path"; -import { execFileSync } from "node:child_process"; // Treat empty strings and un-interpolated "${VAR}" placeholders (which a host // may pass through verbatim when a variable is unset) as "not set" — matching @@ -19,8 +18,6 @@ function val(v) { return t; } -const launchCwd = process.cwd(); - // Resolve where credentials, caches, and config are stored. // CLAUDE_PLUGIN_DATA is the managed lifecycle dir provided by the plugin host. const pluginDataDir = @@ -28,31 +25,6 @@ const pluginDataDir = path.join(os.homedir() || os.tmpdir(), ".glean"); process.env.PLUGIN_DATA_DIR = pluginDataDir; -// Discovered skill files are written under the data dir by default, so the -// skills cache tracks PLUGIN_DATA_DIR instead of being resolved separately. -let skillsBaseDir = path.join(pluginDataDir, "glean-skills-cache"); - -// Opt-in: when USE_CLAUDE_PROJECT_DIR=1, route the skills cache under the launch -// project's .claude/tmp/ so the glean_run skill's allowed-tools Read glob can -// match cache files via a path anchored to the project root. projectDir is the -// git repo root for the launch cwd, falling back to the launch cwd when it is -// not inside a git repo (or git is unavailable). -if (process.env.USE_CLAUDE_PROJECT_DIR === "1") { - let projectDir = launchCwd; - try { - const top = execFileSync( - "git", - ["-C", launchCwd, "rev-parse", "--show-toplevel"], - { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"] }, - ).trim(); - if (top) projectDir = top; - } catch { - /* not a git repo or git missing: keep the launch cwd fallback */ - } - skillsBaseDir = path.join(projectDir, ".claude", "tmp", "glean-skills-cache"); -} -process.env.SKILLS_BASE_DIR = skillsBaseDir; - // Resolve the chat session id host-side. Host-awareness lives here, not in the // plugin: the launcher reads whatever variable this host exposes and exports the // normalized GLEAN_SESSION_ID that the Node bundle reads. Claude Code exposes diff --git a/shared/glean/mcp/tests/find-skills.test.ts b/shared/glean/mcp/tests/find-skills.test.ts index 31c8182..0477491 100644 --- a/shared/glean/mcp/tests/find-skills.test.ts +++ b/shared/glean/mcp/tests/find-skills.test.ts @@ -1,8 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import fs from "node:fs/promises"; -import path from "node:path"; -import os from "node:os"; import { handleFindSkills } from "../src/tools/find-skills.js"; +import { SkillFileCache } from "../src/skill-files.js"; import type { SkillsMap } from "../src/types.js"; function createMockClient(skills: SkillsMap) { @@ -20,19 +18,13 @@ function createMockClient(skills: SkillsMap) { } describe("handleFindSkills", () => { - let tmpDir: string; + let skillFiles: SkillFileCache; - beforeEach(async () => { - tmpDir = await fs.mkdtemp( - path.join(os.tmpdir(), "find-skills-test-"), - ); - }); - - afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }); + beforeEach(() => { + skillFiles = new SkillFileCache(); }); - it("calls find_skills and writes skill files", async () => { + it("calls find_skills and returns an in-memory compatibility index", async () => { const mockClient = createMockClient({ "search-jira": { "SKILL.md": @@ -46,7 +38,7 @@ describe("handleFindSkills", () => { }, }); - const result = await handleFindSkills(mockClient, tmpDir, {}); + const result = await handleFindSkills(mockClient, skillFiles, {}); expect(mockClient.callTool).toHaveBeenCalledWith( expect.objectContaining({ @@ -60,17 +52,15 @@ describe("handleFindSkills", () => { expect(result).toContain(""); expect(result).toContain('name="search-jira"'); - const skillContent = await fs.readFile( - path.join(tmpDir, "search-jira", "SKILL.md"), - "utf-8", - ); - expect(skillContent).toContain("# Search Jira"); + expect(skillFiles.legacy.read("search-jira", ["SKILL.md"])?.files).toEqual({ + "SKILL.md": expect.stringContaining("# Search Jira"), + }); }); it("passes query argument as queries array", async () => { const mockClient = createMockClient({}); - await handleFindSkills(mockClient, tmpDir, { + await handleFindSkills(mockClient, skillFiles, { query: "create a calendar event", }); @@ -87,7 +77,7 @@ describe("handleFindSkills", () => { it("passes queries array when provided", async () => { const mockClient = createMockClient({}); - await handleFindSkills(mockClient, tmpDir, { + await handleFindSkills(mockClient, skillFiles, { queries: ["search emails", "create calendar event"], }); @@ -109,14 +99,14 @@ describe("handleFindSkills", () => { close: vi.fn(), } as any; - const result = await handleFindSkills(mockClient, tmpDir, {}); + const result = await handleFindSkills(mockClient, skillFiles, {}); expect(result).toBe(""); }); it("returns empty XML for no skills", async () => { const mockClient = createMockClient({}); - const result = await handleFindSkills(mockClient, tmpDir, {}); + const result = await handleFindSkills(mockClient, skillFiles, {}); expect(result).toBe(""); }); @@ -127,7 +117,7 @@ describe("handleFindSkills", () => { close: vi.fn(), } as any; - const result = await handleFindSkills(mockClient, tmpDir, {}); + const result = await handleFindSkills(mockClient, skillFiles, {}); expect(result).toBe(""); }); @@ -142,7 +132,7 @@ describe("handleFindSkills", () => { } as any; await expect( - handleFindSkills(mockClient, tmpDir, {}), + handleFindSkills(mockClient, skillFiles, {}), ).rejects.toThrow("backend unavailable"); }); }); diff --git a/shared/glean/mcp/tests/policy-enforce.test.ts b/shared/glean/mcp/tests/policy-enforce.test.ts index 2e3812b..4c35688 100644 --- a/shared/glean/mcp/tests/policy-enforce.test.ts +++ b/shared/glean/mcp/tests/policy-enforce.test.ts @@ -34,6 +34,13 @@ const findSkillsTool: Tool = { name: "find_skills_and_tools", inputSchema: { type: "object", properties: { queries: { type: "array" } } }, }; +const readSkillFilesTool: Tool = { + name: "read_skill_files", + inputSchema: { + type: "object", + properties: { skill_name: { type: "string" }, file_paths: { type: "array" } }, + }, +}; function makeRunTool(): Tool { return { @@ -63,6 +70,7 @@ function advertise(d: Decision) { decision: d, setupTool, findSkillsTool, + readSkillFilesTool, runTool: makeRunTool(), promoted, }); @@ -95,6 +103,7 @@ describe("no policy", () => { }); expect(names(d)).toEqual([ "find_skills_and_tools", + "read_skill_files", "run_tool", "setup", "search", @@ -111,7 +120,7 @@ describe("no policy", () => { supportedFeatures: allSupported, policy: undefined, }); - for (const name of ["setup", "find_skills_and_tools", "run_tool", "search", "chat"]) { + for (const name of ["setup", "find_skills_and_tools", "read_skill_files", "run_tool", "search", "chat"]) { expect(refusal(name, d)).toBeUndefined(); } }); @@ -131,6 +140,7 @@ describe("deactivated", () => { expect(advertise(d).withheld.sort()).toEqual([ "chat", "find_skills_and_tools", + "read_skill_files", "run_tool", "search", ]); @@ -141,7 +151,7 @@ describe("deactivated", () => { }); it("refuses everything else", () => { - for (const name of ["find_skills_and_tools", "run_tool", "search", "chat"]) { + for (const name of ["find_skills_and_tools", "read_skill_files", "run_tool", "search", "chat"]) { expect(refusal(name, d)?.isError).toBe(true); } }); @@ -195,6 +205,7 @@ describe("the remote's upgrade text", () => { expect(names(d)).toEqual([ "find_skills_and_tools", + "read_skill_files", "run_tool", "setup", "search", @@ -208,13 +219,14 @@ describe("the remote's upgrade text", () => { describe("metaTools disabled", () => { const d = decision({ features: { ...allSupported, metaTools: false } }); - it("withdraws both meta tools but keeps setup and promoted tools", () => { + it("withdraws all meta tools but keeps setup and promoted tools", () => { expect(names(d)).toEqual(["setup", "search", "chat"]); - expect(advertise(d).withheld).toEqual(["find_skills_and_tools", "run_tool"]); + expect(advertise(d).withheld).toEqual(["find_skills_and_tools", "read_skill_files", "run_tool"]); }); - it("refuses both by name, and nothing else", () => { + it("refuses all meta tools by name, and nothing else", () => { expect(refusal("find_skills_and_tools", d)?.isError).toBe(true); + expect(refusal("read_skill_files", d)?.isError).toBe(true); expect(refusal("run_tool", d)?.isError).toBe(true); expect(refusal("search", d)).toBeUndefined(); expect(refusal("setup", d)).toBeUndefined(); @@ -225,7 +237,7 @@ describe("toolPromotion disabled", () => { const d = decision({ features: { ...allSupported, toolPromotion: false } }); it("promotes none, and keeps the meta tools", () => { - expect(names(d)).toEqual(["find_skills_and_tools", "run_tool", "setup"]); + expect(names(d)).toEqual(["find_skills_and_tools", "read_skill_files", "run_tool", "setup"]); expect(advertise(d).withheld).toEqual(["search", "chat"]); }); @@ -249,6 +261,7 @@ describe("fileArgs disabled", () => { it("keeps run_tool but drops file_args from its schema", () => { expect(names(d)).toEqual([ "find_skills_and_tools", + "read_skill_files", "run_tool", "setup", "search", @@ -297,6 +310,7 @@ describe("withoutFileArgs", () => { decision: decision({ features: { ...allSupported, fileArgs: false } }), setupTool, findSkillsTool, + readSkillFilesTool, runTool: base, promoted, }); @@ -304,6 +318,7 @@ describe("withoutFileArgs", () => { decision: decision(), setupTool, findSkillsTool, + readSkillFilesTool, runTool: base, promoted, }); @@ -329,7 +344,7 @@ describe("setupClosingLine", () => { it("names the meta tools and the promoted tools when policy allows both", () => { expect(setupClosingLine({ decision: decision(), promoted })).toBe( - "You can now use find_skills_and_tools, run_tool, search, chat.", + "You can now use find_skills_and_tools, read_skill_files, run_tool, search, chat.", ); }); @@ -341,7 +356,7 @@ describe("setupClosingLine", () => { const line = setupClosingLine({ decision: d, promoted }); - expect(line).toBe("You can now use find_skills_and_tools, run_tool."); + expect(line).toBe("You can now use find_skills_and_tools, read_skill_files, run_tool."); for (const name of promoted) { expect(line).not.toContain(name); expect(names(d)).not.toContain(name); @@ -355,6 +370,7 @@ describe("setupClosingLine", () => { expect(line).toBe("You can now use search, chat."); expect(names(d)).not.toContain("find_skills_and_tools"); + expect(names(d)).not.toContain("read_skill_files"); expect(names(d)).not.toContain("run_tool"); }); @@ -362,7 +378,7 @@ describe("setupClosingLine", () => { // neither may leave a dangling reference to a list setup no longer prints. it("names only the meta tools when the remote promotes nothing", () => { expect(setupClosingLine({ decision: decision(), promoted: [] })).toBe( - "You can now use find_skills_and_tools, run_tool.", + "You can now use find_skills_and_tools, read_skill_files, run_tool.", ); }); diff --git a/shared/glean/mcp/tests/read-skill-files.test.ts b/shared/glean/mcp/tests/read-skill-files.test.ts new file mode 100644 index 0000000..60ba690 --- /dev/null +++ b/shared/glean/mcp/tests/read-skill-files.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it, vi } from "vitest"; +import { handleReadSkillFiles } from "../src/tools/read-skill-files.js"; +import { SkillFileCache } from "../src/skill-files.js"; + +function remoteWith(text: string) { + return { + callTool: vi.fn().mockResolvedValue({ + content: [{ type: "text", text }], + }), + } as any; +} + +describe("handleReadSkillFiles", () => { + it("relays the server response and caches tool metadata", async () => { + const response = + `` + + ``; + const remote = remoteWith(response); + const cache = new SkillFileCache(); + + const result = await handleReadSkillFiles(remote, cache, { + skill_name: "search-jira", + file_paths: ["SKILL.md", "tools/search.json"], + }); + + expect(result).toBe(response); + expect(remote.callTool).toHaveBeenCalledWith( + expect.objectContaining({ + name: "read_skill_files", + arguments: { + skill_name: "search-jira", + file_paths: ["SKILL.md", "tools/search.json"], + }, + }), + undefined, + expect.objectContaining({ timeout: expect.any(Number) }), + ); + expect(cache.metadata.get("jira", "search")?.requires_approval).toBe(false); + }); + + it("uses the legacy in-memory response without calling the server", async () => { + const remote = remoteWith("unexpected"); + const cache = new SkillFileCache(); + cache.ingestLegacySkills({ + "search-jira": { + "SKILL.md": "# Search", + "tools/search.json": JSON.stringify({ + name: "search", + server_id: "jira", + requires_approval: false, + }), + }, + }); + + const result = await handleReadSkillFiles(remote, cache, { + skill_name: "search-jira", + file_paths: ["SKILL.md"], + }); + + expect(result).toContain("# Search"); + expect(remote.callTool).not.toHaveBeenCalled(); + }); +}); diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index e1ef2a4..085a37c 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -15,6 +15,7 @@ import { formatArgumentsForFile, } from "../src/tools/approval-args.js"; import type { RunToolPolicy } from "../src/tools/run-tool.js"; +import { ToolMetadataCache } from "../src/skill-files.js"; // The decision every install resolves to today, since production returns no policy. // Passed explicitly at each call site rather than defaulted, so a case that means to @@ -277,17 +278,11 @@ function makeServer(opts: { } async function writeToolJson( - baseDir: string, + metadataCache: ToolMetadataCache, toolName: string, meta: Record, ) { - const toolsDir = path.join(baseDir, "some-skill", "tools"); - await fs.mkdir(toolsDir, { recursive: true }); - await fs.writeFile( - path.join(toolsDir, `${toolName}.json`), - JSON.stringify(meta), - "utf-8", - ); + metadataCache.set({ ...meta, name: toolName }); } // Mirrors the marker the PreToolUse hook writes: /glean-hitl-mode/ @@ -308,6 +303,7 @@ async function writeModeMarker( describe("handleRunTool (HITL)", () => { let tmpDir: string; + let metadataCache: ToolMetadataCache; const baseArgs = { server_id: "composio/jira-pack", tool_name: "jirasearch", @@ -316,6 +312,7 @@ describe("handleRunTool (HITL)", () => { beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "run-tool-hitl-test-")); + metadataCache = new ToolMetadataCache(); }); afterEach(async () => { @@ -327,9 +324,9 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const server = makeServer({ elicitation: false }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(server.elicitInput).not.toHaveBeenCalled(); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -339,9 +336,9 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const server = makeServer({ elicitation: true }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: false }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(server.elicitInput).not.toHaveBeenCalled(); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -353,7 +350,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -365,7 +362,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "decline" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); expect(remote.callTool).not.toHaveBeenCalled(); @@ -376,12 +373,12 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); await handleRunTool( remote, server, - tmpDir, + metadataCache, { server_id: "s", tool_name: "jirasearch", @@ -406,9 +403,9 @@ describe("handleRunTool (HITL)", () => { clientName: "cursor-vscode", elicit, }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); // Cursor is no longer excluded: it gets readOnlyHint like every other // elicitation-capable host, so this prompt is the only approval gate. @@ -428,9 +425,9 @@ describe("handleRunTool (HITL)", () => { clientName: "cursor-vscode", elicit, }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, { + await handleRunTool(remote, server, metadataCache, { ...baseArgs, arguments: { project: "ENG", summary: "ship it" }, }, ALL_ON); @@ -447,9 +444,9 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, { + await handleRunTool(remote, server, metadataCache, { ...baseArgs, arguments: { project: "ENG" }, }, ALL_ON); @@ -478,9 +475,9 @@ describe("handleRunTool (HITL)", () => { clientName: "cursor-vscode", elicit, }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); const text = (result.content[0] as { text: string }).text; expect(result.isError).toBe(true); @@ -507,9 +504,9 @@ describe("handleRunTool (HITL)", () => { clientName: "cursor-vscode", elicit, }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); const text = (result.content[0] as { text: string }).text; expect(text).toContain("cannot tell which"); @@ -538,9 +535,9 @@ describe("handleRunTool (HITL)", () => { clientName: "cursor-vscode", elicit, }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); const text = (result.content[0] as { text: string }).text; expect(result.isError).toBe(true); @@ -560,9 +557,9 @@ describe("handleRunTool (HITL)", () => { ), ); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect((result.content[0] as { text: string }).text).not.toContain("3.15"); expect(remote.callTool).not.toHaveBeenCalled(); @@ -575,12 +572,12 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "false"); const remote = makeRemote(); const server = makeServer({ elicitation: false }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: false }); const result = await handleRunTool( remote, server, - tmpDir, + metadataCache, { ...baseArgs, file_args: { body: "/tmp/whatever.md" } }, { fileArgs: false }, ); @@ -599,12 +596,12 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "false"); const remote = makeRemote(); const server = makeServer({ elicitation: false }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: false }); const result = await handleRunTool( remote, server, - tmpDir, + metadataCache, { ...baseArgs, file_args: { body: path.join(tmpDir, "does-not-exist.md") } }, { fileArgs: false }, ); @@ -618,9 +615,9 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "false"); const remote = makeRemote(); const server = makeServer({ elicitation: false }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: false }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, { + const result = await handleRunTool(remote, server, metadataCache, baseArgs, { fileArgs: false, }); @@ -637,9 +634,9 @@ describe("handleRunTool (HITL)", () => { clientName: "cursor-vscode", elicit, }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); const text = (result.content[0] as { text: string }).text; expect(text).toContain("cancelled by the user"); @@ -653,12 +650,12 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true, description: "Search Jira issues", }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); const [params, options] = elicit.mock.calls[0]; expect(params.message).toContain("Action: jirasearch"); @@ -679,10 +676,10 @@ describe("handleRunTool (HITL)", () => { const request = vi.fn().mockResolvedValue({}); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit, request }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); // Ping fired exactly once for this server, and it is a ping. expect(request).toHaveBeenCalledTimes(1); @@ -696,9 +693,9 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const request = vi.fn().mockResolvedValue({}); const server = makeServer({ elicitation: true, request }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: false }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(request).not.toHaveBeenCalled(); }); @@ -709,16 +706,16 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit.mock.calls[0][1].timeout).toBe(5000); }); it("falls back to the default timeout for invalid HITL_TIMEOUT_MS", async () => { vi.stubEnv("ENABLE_HITL", "true"); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); for (const bad of ["0", "-1", "abc", ""]) { vi.stubEnv("HITL_TIMEOUT_MS", bad); @@ -726,7 +723,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit.mock.calls[0][1].timeout).toBe(300_000); } @@ -737,9 +734,9 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "decline" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(remote.callTool).not.toHaveBeenCalled(); expect((result.content[0] as { text: string }).text).toContain("declined"); @@ -750,9 +747,9 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockRejectedValue(new Error("Request timed out")); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + const result = await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(remote.callTool).not.toHaveBeenCalled(); expect(result.isError).toBe(true); @@ -766,10 +763,10 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); + await writeToolJson(metadataCache, "create_doc", { requires_approval: true }); const bigBody = "| A | B |\n|---|---|\n" + "| x | y |\n".repeat(50); - await handleRunTool(remote, server, tmpDir, { + await handleRunTool(remote, server, metadataCache, { server_id: "s", tool_name: "create_doc", arguments: { title: "Report", body: bigBody }, @@ -797,11 +794,11 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); + await writeToolJson(metadataCache, "create_doc", { requires_approval: true }); const bodyFile = path.join(tmpDir, "draft.md"); await fs.writeFile(bodyFile, "FILE_SOURCED_BODY", "utf-8"); - await handleRunTool(remote, server, tmpDir, { + await handleRunTool(remote, server, metadataCache, { server_id: "s", tool_name: "create_doc", arguments: { title: "Doc" }, @@ -818,14 +815,14 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "false"); const remote = makeRemote(); const server = makeServer({ elicitation: false }); - await writeToolJson(tmpDir, "save_agent", { + await writeToolJson(metadataCache, "save_agent", { requires_approval: false, inputSchema: { properties: { spec: { type: "object" } } }, }); const specFile = path.join(tmpDir, "spec.json"); await fs.writeFile(specFile, '{"name":"my-agent","steps":[1,2]}', "utf-8"); - await handleRunTool(remote, server, tmpDir, { + await handleRunTool(remote, server, metadataCache, { server_id: "default", tool_name: "save_agent", arguments: {}, @@ -845,9 +842,9 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); + await writeToolJson(metadataCache, "create_doc", { requires_approval: true }); - const result = await handleRunTool(remote, server, tmpDir, { + const result = await handleRunTool(remote, server, metadataCache, { server_id: "s", tool_name: "create_doc", arguments: {}, @@ -863,13 +860,13 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("CLAUDE_PLUGIN_DATA", tmpDir); vi.stubEnv("GLEAN_SESSION_ID", "sess-bypass"); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); await writeModeMarker(tmpDir, "sess-bypass", "bypassPermissions"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit).not.toHaveBeenCalled(); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -879,13 +876,13 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("CLAUDE_PLUGIN_DATA", tmpDir); vi.stubEnv("GLEAN_SESSION_ID", "sess-default"); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); await writeModeMarker(tmpDir, "sess-default", "default"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); expect(remote.callTool).toHaveBeenCalledTimes(1); @@ -895,13 +892,13 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("CLAUDE_PLUGIN_DATA", tmpDir); vi.stubEnv("GLEAN_SESSION_ID", "sess-none"); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); // Deliberately write no marker. const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); }); @@ -910,14 +907,14 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("CLAUDE_PLUGIN_DATA", tmpDir); vi.stubEnv("GLEAN_SESSION_ID", "sess-A"); - await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); + await writeToolJson(metadataCache, "jirasearch", { requires_approval: true }); // Another concurrent session opted into bypass; ours did not. await writeModeMarker(tmpDir, "sess-B", "bypassPermissions"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + await handleRunTool(remote, server, metadataCache, baseArgs, ALL_ON); expect(elicit).toHaveBeenCalledTimes(1); // gate preserved for THIS session }); diff --git a/shared/glean/mcp/tests/setup-output.test.ts b/shared/glean/mcp/tests/setup-output.test.ts index 9ca50c1..c0b6cd3 100644 --- a/shared/glean/mcp/tests/setup-output.test.ts +++ b/shared/glean/mcp/tests/setup-output.test.ts @@ -86,7 +86,7 @@ describe("the assembled setup output", () => { expect(text).toContain("No tools are available beyond `setup`."); expect(text).not.toContain("You can now use"); - for (const name of ["find_skills_and_tools", "run_tool", "search", "chat"]) { + for (const name of ["find_skills_and_tools", "read_skill_files", "run_tool", "search", "chat"]) { expect(text).not.toContain(name); } }); @@ -97,7 +97,7 @@ describe("the assembled setup output", () => { ["search", "chat", "employee_search"], ); - expect(text).toContain("You can now use find_skills_and_tools, run_tool."); + expect(text).toContain("You can now use find_skills_and_tools, read_skill_files, run_tool."); // The regression: a withheld feature used to leave these named in "Remote tools: ..." // while being unusable and unadvertised. expect(text).not.toContain("Remote tools:"); @@ -109,7 +109,7 @@ describe("the assembled setup output", () => { it("promotes the remote's tools into the one usable list when policy allows", async () => { const text = await setupText({ features: {} }, ["search", "chat"]); - expect(text).toContain("You can now use find_skills_and_tools, run_tool, search, chat."); + expect(text).toContain("You can now use find_skills_and_tools, read_skill_files, run_tool, search, chat."); expect(text).not.toContain("Remote tools:"); }); }); diff --git a/shared/glean/mcp/tests/skill-files.test.ts b/shared/glean/mcp/tests/skill-files.test.ts new file mode 100644 index 0000000..b5b84ca --- /dev/null +++ b/shared/glean/mcp/tests/skill-files.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from "vitest"; +import { + SkillFileCache, + ToolMetadataCache, + formatLegacySkillIndex, + formatReadSkillFilesResponse, +} from "../src/skill-files.js"; + +describe("ToolMetadataCache", () => { + it("indexes tool metadata from a remote read response", () => { + const cache = new ToolMetadataCache(); + cache.ingestReadResponse( + `\n` + + ` \n` + + ``, + ); + + expect(cache.get("slack", "send")).toEqual({ + name: "send", + server_id: "slack", + requires_approval: true, + inputSchema: { properties: { payload: { type: "object" } } }, + }); + }); + + it("joins a server-split CDATA terminator before parsing JSON", () => { + const cache = new ToolMetadataCache(); + const content = JSON.stringify({ + name: "send", + server_id: "slack", + requires_approval: false, + description: "]]>", + }); + const splitContent = content.replaceAll("]]>", "]]]]>"); + cache.ingestReadResponse( + ``, + ); + + expect(cache.get("slack", "send")?.requires_approval).toBe(false); + }); +}); + +describe("legacy skill compatibility", () => { + const skills = { + "search-jira": { + "SKILL.md": "---\nname: search-jira\ndescription: Search Jira\n---\n# Search", + "tools/search.json": JSON.stringify({ + name: "search", + server_id: "jira", + requires_approval: false, + }), + }, + }; + + it("formats an index with remote file paths, not local paths", () => { + const index = formatLegacySkillIndex(skills); + expect(index).toContain('name="search-jira"'); + expect(index).toContain("SKILL.md"); + expect(index).not.toContain("/tmp/"); + }); + + it("serves requested files from the compatibility cache", () => { + const cache = new SkillFileCache(); + cache.ingestLegacySkills(skills); + const result = cache.legacy.read("search-jira", ["SKILL.md", "missing.md"]); + + expect( + formatReadSkillFilesResponse( + ["SKILL.md", "missing.md"], + result!.files, + result!.availableFiles, + ), + ).toContain('error_reason="not_found"'); + expect(cache.metadata.get("jira", "search")?.requires_approval).toBe(false); + }); +}); diff --git a/shared/glean/mcp/tests/skill-writer.test.ts b/shared/glean/mcp/tests/skill-writer.test.ts deleted file mode 100644 index f70a8a0..0000000 --- a/shared/glean/mcp/tests/skill-writer.test.ts +++ /dev/null @@ -1,336 +0,0 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import fs from "node:fs/promises"; -import path from "node:path"; -import os from "node:os"; -import { - writeSkillsToDisk, - formatAvailableSkillsPrompt, - evictStaleSkills, -} from "../src/skill-writer.js"; -import type { SkillsMap, SkillIndex } from "../src/types.js"; - -describe("writeSkillsToDisk", () => { - let tmpDir: string; - - beforeEach(async () => { - tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "skill-writer-test-")); - }); - - afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }); - }); - - it("writes all files from the flat map", async () => { - const skills: SkillsMap = { - "search-jira": { - "SKILL.md": - "---\nname: search-jira\ndescription: Search Jira issues\n---\n# Search Jira\nUse this skill to search Jira.", - "tools/jirasearch.json": JSON.stringify({ - server_id: "composio/jira-pack", - tool_name: "jirasearch", - description: "Search Jira issues", - input_schema: { - type: "object", - properties: { query: { type: "string" } }, - }, - }), - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index).toHaveLength(1); - expect(index[0].name).toBe("search-jira"); - expect(index[0].description).toBe("Search Jira issues"); - expect(index[0].files).toHaveLength(2); - - expect( - await fs.readFile( - path.join(tmpDir, "search-jira", "SKILL.md"), - "utf-8", - ), - ).toContain("# Search Jira"); - - const toolJson = JSON.parse( - await fs.readFile( - path.join(tmpDir, "search-jira", "tools", "jirasearch.json"), - "utf-8", - ), - ); - expect(toolJson.server_id).toBe("composio/jira-pack"); - expect(toolJson.input_schema.properties.query.type).toBe("string"); - }); - - it("creates nested directories from slash-separated paths", async () => { - const skills: SkillsMap = { - "code-review": { - "SKILL.md": - "---\nname: code-review\ndescription: Review code\n---\n# Code Review", - "templates/review.md": "## Template\nReview checklist", - "config.yaml": "threshold: 0.8", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index[0].files).toHaveLength(3); - expect( - await fs.readFile( - path.join(tmpDir, "code-review", "templates", "review.md"), - "utf-8", - ), - ).toBe("## Template\nReview checklist"); - expect( - await fs.readFile( - path.join(tmpDir, "code-review", "config.yaml"), - "utf-8", - ), - ).toBe("threshold: 0.8"); - }); - - it("parses frontmatter and falls back to directory key when missing", async () => { - const skills: SkillsMap = { - "gcal-event-creation": { - "SKILL.md": - "---\nname: gcal-event-creation\ndescription: Create Google Calendar events\nmetadata:\n author: glean\n---\n# Calendar", - }, - "no-frontmatter": { - "SKILL.md": "# No frontmatter here", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index[0].name).toBe("gcal-event-creation"); - expect(index[0].description).toBe("Create Google Calendar events"); - - expect(index[1].name).toBe("no-frontmatter"); - expect(index[1].description).toBe(""); - }); - - it("parses YAML block scalar descriptions", async () => { - const skills: SkillsMap = { - "block-scalar": { - "SKILL.md": - "---\nname: block-scalar\ndescription: >\n This is a long description\n spanning multiple lines\n---\n# Content", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index[0].name).toBe("block-scalar"); - expect(index[0].description).toContain("This is a long description"); - expect(index[0].description).toContain("spanning multiple lines"); - }); - - it("parses YAML literal block scalar descriptions", async () => { - const skills: SkillsMap = { - "literal-scalar": { - "SKILL.md": - "---\nname: literal-scalar\ndescription: |\n Line one\n Line two\n---\n# Content", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index[0].name).toBe("literal-scalar"); - expect(index[0].description).toContain("Line one"); - expect(index[0].description).toContain("Line two"); - }); - - it("parses YAML with value on next line", async () => { - const skills: SkillsMap = { - "next-line": { - "SKILL.md": - "---\nname: next-line\ndescription:\n The value on next line\n---\n# Content", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index[0].name).toBe("next-line"); - expect(index[0].description).toBe("The value on next line"); - }); - - it("parses frontmatter with CRLF line endings", async () => { - const skills: SkillsMap = { - "crlf-skill": { - "SKILL.md": - "---\r\nname: crlf-skill\r\ndescription: CRLF description\r\n---\r\n# Content", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index[0].name).toBe("crlf-skill"); - expect(index[0].description).toBe("CRLF description"); - }); - - it("prevents path traversal in skill names and file paths", async () => { - const skills: SkillsMap = { - "../../etc/malicious": { - "SKILL.md": "pwned", - }, - "safe-skill": { - "SKILL.md": "---\nname: safe-skill\ndescription: Safe\n---\n# Safe", - "../../etc/passwd": "malicious content", - "legit.md": "safe content", - }, - }; - - const index = await writeSkillsToDisk(skills, tmpDir); - - expect(index).toHaveLength(1); - expect(index[0].name).toBe("safe-skill"); - expect(index[0].files).toHaveLength(2); - expect( - await fs.readFile( - path.join(tmpDir, "safe-skill", "legit.md"), - "utf-8", - ), - ).toBe("safe content"); - await expect( - fs.access(path.join(tmpDir, "..", "etc", "passwd")), - ).rejects.toThrow(); - }); -}); - -describe("evictStaleSkills", () => { - let tmpDir: string; - - beforeEach(async () => { - tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "evict-stale-test-")); - }); - - afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }); - }); - - it("removes directories older than the cutoff and keeps fresh ones", async () => { - const oldDir = path.join(tmpDir, "stale"); - const freshDir = path.join(tmpDir, "fresh"); - await fs.mkdir(oldDir); - await fs.writeFile(path.join(oldDir, "SKILL.md"), "old"); - await fs.mkdir(freshDir); - await fs.writeFile(path.join(freshDir, "SKILL.md"), "new"); - - const eightDaysAgo = Date.now() - 8 * 24 * 60 * 60 * 1000; - await fs.utimes(oldDir, eightDaysAgo / 1000, eightDaysAgo / 1000); - - await evictStaleSkills(tmpDir, 7 * 24 * 60 * 60 * 1000); - - await expect(fs.access(oldDir)).rejects.toThrow(); - await expect(fs.access(freshDir)).resolves.toBeUndefined(); - }); - - it("is a no-op when the base directory does not exist", async () => { - const missing = path.join(tmpDir, "does-not-exist"); - await expect( - evictStaleSkills(missing, 7 * 24 * 60 * 60 * 1000), - ).resolves.toBeUndefined(); - }); - - it("ignores non-directory entries", async () => { - const filePath = path.join(tmpDir, "stray.txt"); - await fs.writeFile(filePath, "not a skill"); - const eightDaysAgo = Date.now() - 8 * 24 * 60 * 60 * 1000; - await fs.utimes(filePath, eightDaysAgo / 1000, eightDaysAgo / 1000); - - await evictStaleSkills(tmpDir, 7 * 24 * 60 * 60 * 1000); - - await expect(fs.access(filePath)).resolves.toBeUndefined(); - }); - - it("keeps a directory whose mtime is exactly at the cutoff (strict <)", async () => { - const boundaryDir = path.join(tmpDir, "boundary"); - await fs.mkdir(boundaryDir); - const maxAgeMs = 7 * 24 * 60 * 60 * 1000; - // Truncate to whole seconds so utimes stores the mtime exactly — filesystems - // that only support second granularity would otherwise truncate the value - // below the cutoff and incorrectly evict the directory. - const now = Math.floor(Date.now() / 1000) * 1000; - const cutoffSec = (now - maxAgeMs) / 1000; - await fs.utimes(boundaryDir, cutoffSec, cutoffSec); - - await evictStaleSkills(tmpDir, maxAgeMs, undefined, now); - - await expect(fs.access(boundaryDir)).resolves.toBeUndefined(); - }); - - it("invokes the log callback for each evicted skill", async () => { - const staleDir = path.join(tmpDir, "stale-logged"); - await fs.mkdir(staleDir); - const eightDaysAgo = Date.now() - 8 * 24 * 60 * 60 * 1000; - await fs.utimes(staleDir, eightDaysAgo / 1000, eightDaysAgo / 1000); - - const calls: { label: string; detail?: Record }[] = []; - await evictStaleSkills( - tmpDir, - 7 * 24 * 60 * 60 * 1000, - (label, detail) => { - calls.push({ label, detail }); - }, - ); - - expect( - calls.some( - (c) => - c.label === "evict-stale-skill" && - c.detail?.skill === "stale-logged", - ), - ).toBe(true); - }); -}); - -describe("formatAvailableSkillsPrompt", () => { - it("formats skills with file references (no instructions block)", () => { - const index: SkillIndex[] = [ - { - name: "search-jira", - description: "Search Jira issues", - skillDir: "/tmp/skills/search-jira", - files: [ - "/tmp/skills/search-jira/SKILL.md", - "/tmp/skills/search-jira/tools/jirasearch.json", - ], - }, - { - name: "create-event", - description: "Create calendar events", - skillDir: "/tmp/skills/create-event", - files: ["/tmp/skills/create-event/SKILL.md"], - }, - ]; - - const result = formatAvailableSkillsPrompt(index); - - expect(result).toContain(""); - expect(result).not.toContain(""); - expect(result).not.toContain("Browse the skills below"); - expect(result).toContain('name="search-jira"'); - expect(result).toContain('description="Search Jira issues"'); - expect(result).toContain('path="/tmp/skills/search-jira/SKILL.md"'); - expect(result).not.toContain( - 'path="/tmp/skills/search-jira/tools/jirasearch.json"', - ); - expect(result).toContain('name="create-event"'); - expect(result).toContain(""); - }); - - it("escapes XML special characters", () => { - const index: SkillIndex[] = [ - { - name: "test", - description: 'Has "quotes" & ', - skillDir: "/tmp/test", - files: [], - }, - ]; - - const result = formatAvailableSkillsPrompt(index); - - expect(result).toContain("&"); - expect(result).toContain("<angles>"); - expect(result).toContain(""quotes""); - }); -}); diff --git a/shared/glean/mcp/tests/version.test.ts b/shared/glean/mcp/tests/version.test.ts index f64f268..c34b375 100644 --- a/shared/glean/mcp/tests/version.test.ts +++ b/shared/glean/mcp/tests/version.test.ts @@ -64,7 +64,6 @@ describe("plugin version", () => { env: { ...process.env, CLAUDE_PLUGIN_DATA: path.join(staged, "plugin-data"), - SKILLS_BASE_DIR: path.join(staged, "skills"), }, stdio: "pipe", });