diff --git a/src/cli/commands/generate-skills.ts b/src/cli/commands/generate-skills.ts index b1a548f..8c5c276 100644 --- a/src/cli/commands/generate-skills.ts +++ b/src/cli/commands/generate-skills.ts @@ -214,6 +214,7 @@ export const handleGenerateSkills = async (args: string[]): Promise => { // Get full schemas for each tool (already filtered by access control) const allowedToolNames = new Set(tools.map((tool) => tool.name)); const schemas: SchemaOutput[] = discovery.schemas.filter((schema) => allowedToolNames.has(schema.tool)); + const cachedSchemas = discovery.cachedSchemas.filter((schema) => allowedToolNames.has(schema.name)); // Group tools by prefix const groups = detectPrefixGroups(schemas, serviceName); @@ -222,8 +223,9 @@ export const handleGenerateSkills = async (args: string[]): Promise => { const descriptions = tools.map((t) => t.description); const triggerKeywords = extractTriggerKeywords(serviceName, descriptions); - // Compute schema hash from tool names + descriptions for drift detection - const schemaHash = await computeSchemaHash(tools); + // Compute schema hash from the same full-description surface that `skills list` + // compares against. The rendered tool table may use truncated descriptions. + const schemaHash = await computeSchemaHash(cachedSchemas); // H3: Only update generatedAt when the schema hash actually changes const outputDir = resolveOutputDir(serviceName, outputFlag); diff --git a/src/cli/commands/skills.ts b/src/cli/commands/skills.ts index 8d87bd4..240d9c9 100644 --- a/src/cli/commands/skills.ts +++ b/src/cli/commands/skills.ts @@ -4,6 +4,7 @@ */ import { loadConfig } from "../../config/index.ts"; import { listCachedServices, readCacheRaw } from "../../cache/index.ts"; +import { extractPolicy, filterTools } from "../../access/filter.ts"; import { resolveOutputDir } from "../../generation/file-manager.ts"; import { computeSchemaHash } from "../../generation/skill-hash.ts"; import { validateIdentifier } from "../../validation/pipelines.ts"; @@ -103,10 +104,13 @@ async function handleSkillsList(args: string[]): Promise { if (!exists) { const cached = cachedServices.includes(name) ? await readCacheRaw(name) : null; + const visibleCachedTools = cached + ? filterTools(cached.tools, extractPolicy(config.services[name]!)) + : null; statuses.push({ service: name, status: "missing", - cachedToolCount: cached?.tools.length, + cachedToolCount: visibleCachedTools?.length, }); continue; } @@ -118,11 +122,14 @@ async function handleSkillsList(args: string[]): Promise { const existingHash = hashMatch?.[1]; const cached = cachedServices.includes(name) ? await readCacheRaw(name) : null; + const visibleCachedTools = cached + ? filterTools(cached.tools, extractPolicy(config.services[name]!)) + : null; let isStale = false; let toolCount: number | undefined; - if (cached) { - const cacheHash = await computeSchemaHash(cached.tools); + if (visibleCachedTools) { + const cacheHash = await computeSchemaHash(visibleCachedTools); isStale = existingHash !== cacheHash; } @@ -134,7 +141,7 @@ async function handleSkillsList(args: string[]): Promise { service: name, status: isStale ? "stale" : "generated", toolCount, - cachedToolCount: cached?.tools.length, + cachedToolCount: visibleCachedTools?.length, schemaHash: existingHash, path: skillDir, }); diff --git a/src/generation/file-manager.ts b/src/generation/file-manager.ts index 7aeaae3..28eff3a 100644 --- a/src/generation/file-manager.ts +++ b/src/generation/file-manager.ts @@ -34,6 +34,101 @@ export function resolveOutputDir( /** Tolerant regex for AUTO-GENERATED markers */ const MARKER_START_RE = //; const MARKER_END_RE = //; +const FRONTMATTER_RE = /^---\n[\s\S]*?\n---\n?/; + +interface FrontmatterBlock { + key: string; + lines: string[]; +} + +function splitFrontmatterBlocks(frontmatter: string): FrontmatterBlock[] { + const body = frontmatter + .replace(/^---\n/, "") + .replace(/\n---\n?$/, ""); + const blocks: FrontmatterBlock[] = []; + + for (const line of body.split("\n")) { + const keyMatch = line.match(/^([A-Za-z0-9_-]+):/); + if (keyMatch) { + blocks.push({ key: keyMatch[1]!, lines: [line] }); + } else if (blocks.length > 0) { + blocks[blocks.length - 1]!.lines.push(line); + } + } + + return blocks; +} + +function triggerValues(block: FrontmatterBlock | undefined): string[] { + if (!block) return []; + + const firstLine = block.lines[0] ?? ""; + const inlineArrayMatch = firstLine.match(/^[A-Za-z0-9_-]+:\s*\[(.*)\]\s*$/); + if (inlineArrayMatch) { + return inlineArrayMatch[1]! + .split(",") + .map((value) => value.trim().replace(/^['"]|['"]$/g, "")) + .filter(Boolean); + } + + const inlineScalarMatch = firstLine.match(/^[A-Za-z0-9_-]+:\s*(\S.*)$/); + if (inlineScalarMatch) { + return [inlineScalarMatch[1]!.trim().replace(/^['"]|['"]$/g, "")].filter(Boolean); + } + + return block.lines + .slice(1) + .map((line) => line.match(/^\s*-\s+(.+)\s*$/)?.[1]) + .filter((value): value is string => Boolean(value)); +} + +function mergeFrontmatter(existingFrontmatter: string, generatedFrontmatter: string): string { + const existingBlocks = splitFrontmatterBlocks(existingFrontmatter); + const generatedBlocks = splitFrontmatterBlocks(generatedFrontmatter); + const generatedKeys = new Set(generatedBlocks.map((block) => block.key)); + const existingByKey = new Map(existingBlocks.map((block) => [block.key, block])); + const merged: string[] = ["---"]; + + for (const generatedBlock of generatedBlocks) { + if (generatedBlock.key === "triggers") { + const values = [ + ...triggerValues(generatedBlock), + ...triggerValues(existingByKey.get("triggers")), + ]; + const deduped = [...new Set(values)]; + merged.push("triggers:"); + for (const value of deduped) { + merged.push(` - ${value}`); + } + } else { + merged.push(...generatedBlock.lines); + } + } + + for (const existingBlock of existingBlocks) { + if (!generatedKeys.has(existingBlock.key)) { + merged.push(...existingBlock.lines); + } + } + + merged.push("---"); + return merged.join("\n") + "\n"; +} + +function mergeGeneratedFrontmatter(existing: string, generated: string): string { + const generatedFrontmatter = generated.match(FRONTMATTER_RE)?.[0]; + if (!generatedFrontmatter) { + return existing; + } + + if (FRONTMATTER_RE.test(existing)) { + return existing.replace(FRONTMATTER_RE, (existingFrontmatter) => + mergeFrontmatter(existingFrontmatter, generatedFrontmatter) + ); + } + + return generatedFrontmatter + existing; +} /** * Merge generated content into existing file content. @@ -63,7 +158,10 @@ export function mergeContent(existing: string, generated: string): string { } // Replace between markers, preserving user content before and after - const beforeMarker = existing.slice(0, startMatch.index!); + const beforeMarker = mergeGeneratedFrontmatter( + existing.slice(0, startMatch.index!), + generated, + ); const afterMarker = existing.slice(endMatch.index! + endMatch[0]!.length); return beforeMarker + autoContent + afterMarker; diff --git a/tests/cli/skills.test.ts b/tests/cli/skills.test.ts index f19d235..f52c8b8 100644 --- a/tests/cli/skills.test.ts +++ b/tests/cli/skills.test.ts @@ -290,6 +290,57 @@ describe("skills list", () => { } }); + test("uses access-filtered cache surface for stale detection", async () => { + const tools = [makeTool("allowed_tool", "Allowed"), makeTool("blocked_tool", "Blocked")]; + await writeCache("test-svc", tools); + + const { computeSchemaHash } = await import("../../src/generation/skill-hash.ts"); + const expectedHash = await computeSchemaHash([tools[0]!]); + + const skillContent = [ + "---", + "name: test-svc", + "tool_count: 1", + "generated_at: 2025-01-01T00:00:00.000Z", + `schema_hash: ${expectedHash}`, + "triggers:", + " - test-svc", + "---", + "", + "# test-svc", + ].join("\n"); + await writeSkillFile("test-svc", skillContent); + + const origConfig = process.env.MCP2CLI_CONFIG; + const configPath = join(testDir, "services.json"); + await Bun.write(configPath, JSON.stringify({ + services: { + "test-svc": { + backend: "stdio", + command: "echo", + args: [], + allowTools: ["allowed_*"], + }, + }, + })); + process.env.MCP2CLI_CONFIG = configPath; + + try { + const { stdout } = await captureSkills(["list"]); + expect(stdout).toContain("test-svc"); + expect(stdout).toContain("ok"); + expect(stdout).toContain("1 tools"); + expect(stdout).not.toContain("stale"); + expect(stdout).not.toContain("cache has 2"); + } finally { + if (origConfig !== undefined) { + process.env.MCP2CLI_CONFIG = origConfig; + } else { + delete process.env.MCP2CLI_CONFIG; + } + } + }); + // L11: Test --json output test("outputs JSON when --json flag is passed via args", async () => { const origConfig = process.env.MCP2CLI_CONFIG; diff --git a/tests/generation/file-manager.test.ts b/tests/generation/file-manager.test.ts index 41c5fc0..256af5f 100644 --- a/tests/generation/file-manager.test.ts +++ b/tests/generation/file-manager.test.ts @@ -133,6 +133,42 @@ describe("mergeContent", () => { expect(result).toContain("Before"); expect(result).toContain("After"); }); + + test("preserves inline trigger array values when merging frontmatter", () => { + const existing = [ + "---", + "name: old-skill", + "triggers: [custom-trigger, \"quoted trigger\"]", + "x-owner: human", + "---", + "# Custom header", + "", + "old", + "", + ].join("\n"); + + const generated = [ + "---", + "name: generated-skill", + "triggers:", + " - generated-trigger", + "schema_hash: abc123", + "---", + "", + "new", + "", + ].join("\n"); + + const result = mergeContent(existing, generated); + expect(result).toContain("name: generated-skill"); + expect(result).toContain("schema_hash: abc123"); + expect(result).toContain(" - generated-trigger"); + expect(result).toContain(" - custom-trigger"); + expect(result).toContain(" - quoted trigger"); + expect(result).toContain("x-owner: human"); + expect(result).toContain("new"); + expect(result).not.toContain("old"); + }); }); // -- planFileWrites -- diff --git a/tests/integration/generate-skills.test.ts b/tests/integration/generate-skills.test.ts index b488201..a49e984 100644 --- a/tests/integration/generate-skills.test.ts +++ b/tests/integration/generate-skills.test.ts @@ -248,6 +248,50 @@ describe("generate-skills integration", () => { expect(afterContent).toContain("AUTO-GENERATED:END"); }, 30_000); + test("conflict merge refreshes generated frontmatter metadata", async () => { + const { env, outputDir } = await setupTestEnv(); + + runCli( + ["generate-skills", "mock-server", `--output=${outputDir}`], + env, + ); + + const skillPath = join(outputDir, "SKILL.md"); + const original = await Bun.file(skillPath).text(); + const oldFrontmatter = [ + "---", + "name: mock-server", + "description: MCP tools for mock-server", + "triggers:", + " - mock-server", + " - custom-trigger", + "x-owner: hand-edited", + "---", + "", + ].join("\n"); + const withoutMetadata = original.replace(/^---\n[\s\S]*?\n---\n\n/, oldFrontmatter); + await Bun.write( + skillPath, + withoutMetadata + "\n## My Custom Notes\n\nThis should be preserved.\n", + ); + + const result = runCli( + ["generate-skills", "mock-server", `--output=${outputDir}`, "--conflict=merge"], + env, + ); + + expect(result.exitCode).toBe(0); + + const afterContent = await Bun.file(skillPath).text(); + expect(afterContent).toContain("tool_count: 3"); + expect(afterContent).toMatch(/^generated_at:\s*\S+/m); + expect(afterContent).toMatch(/^schema_hash:\s*[0-9a-f]{16}/m); + expect(afterContent).toContain(" - custom-trigger"); + expect(afterContent).toContain("x-owner: hand-edited"); + expect(afterContent).toContain("My Custom Notes"); + expect(afterContent).toContain("This should be preserved."); + }, 30_000); + test("missing service arg returns structured validation error", async () => { const { env } = await setupTestEnv();