From d0fc8a334815eecacf84ebb5a6991be2d136f988 Mon Sep 17 00:00:00 2001 From: Mikkel Georgsen Date: Wed, 1 Apr 2026 11:19:57 +0200 Subject: [PATCH] [nexus] feat(19-01): unit tests for adapter-aware install/uninstall and Hermes dual-source - skill-registry-adapter-install.test.ts: 9 tests covering install/uninstall/rollback/assignGroup/removeGroup - hermes-dual-source.test.ts: 7 tests covering syncHermesNativeSkills idempotency and listAgentSkills object shape - Fix skill-registry-install.test.ts: update uninstall() callers to pass agentSkillsDir (new required param) - Fix removeGroup() bug: removed incorrect 'individualSkills' guard that prevented file removal for group-installed skills (rule 1 auto-fix: group-installed skills were never removed because they appeared in agentSkills with no way to distinguish from direct installs) - All 16 new tests pass, all existing tests still pass --- .../src/__tests__/hermes-dual-source.test.ts | 214 +++++++++++++ .../skill-registry-adapter-install.test.ts | 295 ++++++++++++++++++ .../__tests__/skill-registry-install.test.ts | 4 +- server/src/services/skill-registry-groups.ts | 11 +- 4 files changed, 513 insertions(+), 11 deletions(-) create mode 100644 server/src/__tests__/hermes-dual-source.test.ts create mode 100644 server/src/__tests__/skill-registry-adapter-install.test.ts diff --git a/server/src/__tests__/hermes-dual-source.test.ts b/server/src/__tests__/hermes-dual-source.test.ts new file mode 100644 index 00000000..a8ec2ba1 --- /dev/null +++ b/server/src/__tests__/hermes-dual-source.test.ts @@ -0,0 +1,214 @@ +/** + * Tests for Hermes dual-source skill tracking. + * Verifies syncHermesNativeSkills and listAgentSkills typed return shape. + * + * Uses real temp directories instead of mocking readdir (ESM limitation prevents + * direct spying on node:fs/promises named exports in Vitest). + * + * Covers: HERM-01, HERM-02, HERM-03 + */ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtemp, rm, mkdir } from "node:fs/promises"; +import path from "node:path"; +import os from "node:os"; +import { getSkillRegistryDb, resetSkillRegistryDb } from "../services/skill-registry-db.js"; +import { skills, agentSkills } from "../services/skill-registry-schema.js"; +import { skillRegistryService } from "../services/skill-registry.js"; +import { skillGroupService } from "../services/skill-registry-groups.js"; +import { eq } from "drizzle-orm"; + +// --------------------------------------------------------------------------- +// Test helpers +// --------------------------------------------------------------------------- + +let tmpHome: string; +let fakeHermesSkillsDir: string; +let originalHome: string | undefined; + +beforeEach(async () => { + tmpHome = await mkdtemp(path.join(os.tmpdir(), "nexus-hermes-test-")); + process.env.PAPERCLIP_HOME = tmpHome; + + // Override HOME so syncHermesNativeSkills reads our fake ~/.hermes/skills/ + originalHome = process.env.HOME; + process.env.HOME = tmpHome; + + fakeHermesSkillsDir = path.join(tmpHome, ".hermes", "skills"); + resetSkillRegistryDb(); +}); + +afterEach(async () => { + resetSkillRegistryDb(); + delete process.env.PAPERCLIP_HOME; + if (originalHome !== undefined) { + process.env.HOME = originalHome; + } else { + delete process.env.HOME; + } + await rm(tmpHome, { recursive: true, force: true }); +}); + +/** Create fake skill directories inside ~/.hermes/skills/ */ +async function createFakeHermesSkills(skillNames: string[]): Promise { + await mkdir(fakeHermesSkillsDir, { recursive: true }); + for (const name of skillNames) { + await mkdir(path.join(fakeHermesSkillsDir, name), { recursive: true }); + } +} + +// --------------------------------------------------------------------------- +// syncHermesNativeSkills +// --------------------------------------------------------------------------- + +describe("syncHermesNativeSkills()", () => { + it("Test 1: creates skills stub rows with sourceId 'hermes-native'", async () => { + await createFakeHermesSkills(["code-reviewer", "test-writer"]); + + const svc = skillRegistryService(); + await svc.syncHermesNativeSkills("agent-hermes-1"); + + const db = await getSkillRegistryDb(); + const skillRows = await db.select().from(skills) + .where(eq(skills.sourceId, "hermes-native")); + + expect(skillRows.length).toBe(2); + expect(skillRows.map((r) => r.id).sort()).toEqual([ + "hermes-native/code-reviewer", + "hermes-native/test-writer", + ].sort()); + expect(skillRows.every((r) => r.sourceId === "hermes-native")).toBe(true); + }); + + it("Test 2: creates agentSkills rows with source 'native'", async () => { + await createFakeHermesSkills(["code-reviewer"]); + + const svc = skillRegistryService(); + await svc.syncHermesNativeSkills("agent-hermes-2"); + + const db = await getSkillRegistryDb(); + const agentSkillRows = await db.select().from(agentSkills) + .where(eq(agentSkills.agentId, "agent-hermes-2")); + + expect(agentSkillRows.length).toBe(1); + expect(agentSkillRows[0]!.skillId).toBe("hermes-native/code-reviewer"); + expect(agentSkillRows[0]!.source).toBe("native"); + }); + + it("Test 3: is idempotent — running twice does not duplicate rows", async () => { + await createFakeHermesSkills(["code-reviewer"]); + + const svc = skillRegistryService(); + // Run twice + await svc.syncHermesNativeSkills("agent-hermes-3"); + await svc.syncHermesNativeSkills("agent-hermes-3"); + + const db = await getSkillRegistryDb(); + const agentSkillRows = await db.select().from(agentSkills) + .where(eq(agentSkills.agentId, "agent-hermes-3")); + + // Should only have ONE row despite calling sync twice + expect(agentSkillRows.length).toBe(1); + }); + + it("Test 4: handles missing ~/.hermes/skills/ gracefully (no throw)", async () => { + // Do NOT create the hermes skills dir — simulate it not existing + + const svc = skillRegistryService(); + // Must not throw + await expect(svc.syncHermesNativeSkills("agent-hermes-4")).resolves.toBeUndefined(); + + const db = await getSkillRegistryDb(); + const agentSkillRows = await db.select().from(agentSkills) + .where(eq(agentSkills.agentId, "agent-hermes-4")); + expect(agentSkillRows.length).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// listAgentSkills — typed object shape +// --------------------------------------------------------------------------- + +describe("listAgentSkills() — returns objects with source field", () => { + it("Test 5: returns Array<{skillId, source, installedAt}> not string[]", async () => { + const db = await getSkillRegistryDb(); + const agentId = "agent-shape-test"; + const now = Date.now(); + + // Insert a managed skill directly + await db.insert(agentSkills).values({ + agentId, + skillId: "nexus-skills/code-review", + installedAt: now, + source: "managed", + }); + + const grpSvc = skillGroupService(); + const result = await grpSvc.listAgentSkills(agentId); + + expect(Array.isArray(result)).toBe(true); + expect(result.length).toBe(1); + + const entry = result[0]!; + // Must have the 3-field object shape + expect(typeof entry.skillId).toBe("string"); + expect(typeof entry.source).toBe("string"); + expect(typeof entry.installedAt).toBe("number"); + + // Must NOT be a plain string (old API) + expect(typeof entry).toBe("object"); + }); + + it("Test 6: returns skills with correct source values", async () => { + const db = await getSkillRegistryDb(); + const agentId = "agent-source-values"; + const now = Date.now(); + + await db.insert(agentSkills).values([ + { agentId, skillId: "nexus-skills/managed-skill", installedAt: now, source: "managed" }, + { agentId, skillId: "hermes-native/native-skill", installedAt: now, source: "native" }, + ]); + + const grpSvc = skillGroupService(); + const result = await grpSvc.listAgentSkills(agentId); + + expect(result.length).toBe(2); + const managedEntry = result.find((r) => r.skillId === "nexus-skills/managed-skill"); + const nativeEntry = result.find((r) => r.skillId === "hermes-native/native-skill"); + + expect(managedEntry?.source).toBe("managed"); + expect(nativeEntry?.source).toBe("native"); + }); + + it("Test 7: listAgentSkills includes both managed and native skills for a Hermes agent", async () => { + await createFakeHermesSkills(["native-skill"]); + + const agentId = "agent-hermes-combined"; + const db = await getSkillRegistryDb(); + const now = Date.now(); + + // Insert a managed skill + await db.insert(agentSkills).values({ + agentId, + skillId: "nexus-skills/managed", + installedAt: now, + source: "managed", + }); + + // Sync native skills + const svc = skillRegistryService(); + await svc.syncHermesNativeSkills(agentId); + + const grpSvc = skillGroupService(); + const result = await grpSvc.listAgentSkills(agentId); + + expect(result.length).toBe(2); + + const managed = result.filter((r) => r.source === "managed"); + const native = result.filter((r) => r.source === "native"); + + expect(managed.length).toBe(1); + expect(native.length).toBe(1); + expect(managed[0]!.skillId).toBe("nexus-skills/managed"); + expect(native[0]!.skillId).toBe("hermes-native/native-skill"); + }); +}); diff --git a/server/src/__tests__/skill-registry-adapter-install.test.ts b/server/src/__tests__/skill-registry-adapter-install.test.ts new file mode 100644 index 00000000..9cc1e99c --- /dev/null +++ b/server/src/__tests__/skill-registry-adapter-install.test.ts @@ -0,0 +1,295 @@ +/** + * Tests for adapter-aware install/uninstall/rollback/assignGroup/removeGroup. + * Verifies that all file operations use the caller-supplied agentSkillsDir + * rather than any hardcoded path. + * + * Covers: INST-01, INST-02, INST-03, INST-04 + */ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtemp, rm, mkdir, writeFile, readdir } from "node:fs/promises"; +import { existsSync } from "node:fs"; +import path from "node:path"; +import os from "node:os"; +import { getSkillRegistryDb, resetSkillRegistryDb } from "../services/skill-registry-db.js"; +import { skills, skillVersions, skillFiles, agentSkills } from "../services/skill-registry-schema.js"; +import { skillRegistryService } from "../services/skill-registry.js"; +import { skillGroupService } from "../services/skill-registry-groups.js"; + +// --------------------------------------------------------------------------- +// Test helpers +// --------------------------------------------------------------------------- + +let tmpHome: string; +let tmpAgentSkillsDir: string; + +beforeEach(async () => { + tmpHome = await mkdtemp(path.join(os.tmpdir(), "nexus-adapter-install-test-")); + tmpAgentSkillsDir = path.join(tmpHome, "agent-skills"); + await mkdir(tmpAgentSkillsDir, { recursive: true }); + process.env.PAPERCLIP_HOME = tmpHome; + resetSkillRegistryDb(); +}); + +afterEach(async () => { + resetSkillRegistryDb(); + delete process.env.PAPERCLIP_HOME; + await rm(tmpHome, { recursive: true, force: true }); +}); + +async function seedSkillWithVersion(opts: { + skillId: string; + sourceId: string; + versionId: string; + cacheDir?: string; + fileKind?: string; +}): Promise { + const db = await getSkillRegistryDb(); + const now = Date.now(); + await db.insert(skills).values({ + id: opts.skillId, + sourceId: opts.sourceId, + name: "Test Skill", + description: null, + sourceUrl: null, + activeVersionId: null, + removedAt: null, + createdAt: now, + updatedAt: now, + }); + await db.insert(skillVersions).values({ + id: opts.versionId, + skillId: opts.skillId, + version: "abc123", + fetchedAt: now, + cacheDir: opts.cacheDir ?? null, + }); + await db.insert(skillFiles).values({ + id: `file-${opts.versionId}`, + versionId: opts.versionId, + path: "SKILL.md", + kind: opts.fileKind ?? "skill", + sizeBytes: 100, + }); +} + +async function createFakeCacheDir(cacheDir: string, content = "# Test Skill"): Promise { + await mkdir(cacheDir, { recursive: true }); + await writeFile(path.join(cacheDir, "SKILL.md"), content, "utf-8"); +} + +// --------------------------------------------------------------------------- +// install() — writes to provided agentSkillsDir +// --------------------------------------------------------------------------- + +describe("install() — adapter-aware path", () => { + it("Test 1: writes skill files to the provided agentSkillsDir, not a hardcoded path", async () => { + const skillId = "nexus-skills/typescript-review"; + const versionId = `${skillId}@abc123`; + const cacheDir = path.join(tmpHome, "cache", "typescript-review"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "nexus-skills", versionId, cacheDir }); + + const svc = skillRegistryService(); + const result = await svc.install(skillId, tmpAgentSkillsDir); + + expect(result.type).toBe("installed"); + if (result.type === "installed") { + // Files must land inside tmpAgentSkillsDir, not any hardcoded path + expect(result.targetDir).toContain(tmpAgentSkillsDir); + expect(existsSync(path.join(result.targetDir, "SKILL.md"))).toBe(true); + } + }); + + it("Test 2: slug (last segment of skillId) is used as subdirectory name", async () => { + const skillId = "nexus-skills/code-review"; + const versionId = `${skillId}@sha1`; + const cacheDir = path.join(tmpHome, "cache", "code-review"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "nexus-skills", versionId, cacheDir }); + + const svc = skillRegistryService(); + const result = await svc.install(skillId, tmpAgentSkillsDir); + + expect(result.type).toBe("installed"); + if (result.type === "installed") { + expect(result.targetDir).toBe(path.join(tmpAgentSkillsDir, "code-review")); + } + }); +}); + +// --------------------------------------------------------------------------- +// uninstall() — removes files AND soft-deletes DB row +// --------------------------------------------------------------------------- + +describe("uninstall() — file removal + soft-delete", () => { + it("Test 3: removes skill directory from disk", async () => { + const skillId = "nexus-skills/code-review"; + const versionId = `${skillId}@abc123`; + const cacheDir = path.join(tmpHome, "cache", "code-review"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "nexus-skills", versionId, cacheDir }); + + const svc = skillRegistryService(); + // First install so files are on disk + await svc.install(skillId, tmpAgentSkillsDir); + const targetDir = path.join(tmpAgentSkillsDir, "code-review"); + expect(existsSync(targetDir)).toBe(true); + + // Now uninstall — should remove files + await svc.uninstall(skillId, tmpAgentSkillsDir); + expect(existsSync(targetDir)).toBe(false); + }); + + it("Test 4: soft-deletes the DB row (sets removedAt)", async () => { + const skillId = "nexus-skills/code-review"; + const versionId = `${skillId}@abc123`; + const cacheDir = path.join(tmpHome, "cache", "code-review"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "nexus-skills", versionId, cacheDir }); + + const svc = skillRegistryService(); + const before = Date.now(); + await svc.uninstall(skillId, tmpAgentSkillsDir); + const after = Date.now(); + + const db = await getSkillRegistryDb(); + const { eq } = await import("drizzle-orm"); + const rows = await db.select().from(skills).where(eq(skills.id, skillId)); + expect(rows[0]?.removedAt).toBeGreaterThanOrEqual(before); + expect(rows[0]?.removedAt).toBeLessThanOrEqual(after); + }); + + it("Test 5: uninstall with non-existent directory does not throw (force: true)", async () => { + const skillId = "nexus-skills/missing-skill"; + await seedSkillWithVersion({ + skillId, + sourceId: "nexus-skills", + versionId: `${skillId}@sha1`, + }); + + const svc = skillRegistryService(); + // targetDir never existed — should not throw + await expect(svc.uninstall(skillId, tmpAgentSkillsDir)).resolves.toBeUndefined(); + }); + + it("Test 6: uninstall removes files from the correct path (slug-based subdirectory)", async () => { + const skillId = "my-source/my-skill-name"; + const versionId = `${skillId}@abc123`; + const cacheDir = path.join(tmpHome, "cache", "my-skill-name"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "my-source", versionId, cacheDir }); + + const svc = skillRegistryService(); + await svc.install(skillId, tmpAgentSkillsDir); + + // Confirm the correct subdirectory was created + const expectedDir = path.join(tmpAgentSkillsDir, "my-skill-name"); + expect(existsSync(expectedDir)).toBe(true); + + await svc.uninstall(skillId, tmpAgentSkillsDir); + expect(existsSync(expectedDir)).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// rollback() — restores files to provided agentSkillsDir +// --------------------------------------------------------------------------- + +describe("rollback() — restores to provided agentSkillsDir", () => { + it("Test 7: restores files to the provided agentSkillsDir (not a hardcoded path)", async () => { + const skillId = "nexus-skills/code-review"; + const v1Id = `${skillId}@v1`; + const v2Id = `${skillId}@v2`; + const v1CacheDir = path.join(tmpHome, "cache", skillId, "v1"); + const v2CacheDir = path.join(tmpHome, "cache", skillId, "v2"); + + await createFakeCacheDir(v1CacheDir, "# Version 1"); + await createFakeCacheDir(v2CacheDir, "# Version 2"); + + const db = await getSkillRegistryDb(); + const now = Date.now(); + await db.insert(skills).values({ + id: skillId, sourceId: "nexus-skills", name: "Code Review", + description: null, sourceUrl: null, activeVersionId: v2Id, + removedAt: null, createdAt: now, updatedAt: now, + }); + await db.insert(skillVersions).values({ id: v1Id, skillId, version: "v1", fetchedAt: now - 1000, cacheDir: v1CacheDir }); + await db.insert(skillVersions).values({ id: v2Id, skillId, version: "v2", fetchedAt: now, cacheDir: v2CacheDir }); + await db.insert(skillFiles).values({ id: "f1", versionId: v1Id, path: "SKILL.md", kind: "skill", sizeBytes: 12 }); + await db.insert(skillFiles).values({ id: "f2", versionId: v2Id, path: "SKILL.md", kind: "skill", sizeBytes: 12 }); + + // Pre-install v2 + const targetDir = path.join(tmpAgentSkillsDir, "code-review"); + await mkdir(targetDir, { recursive: true }); + await writeFile(path.join(targetDir, "SKILL.md"), "# Version 2", "utf-8"); + + // Rollback to v1 using provided agentSkillsDir + const svc = skillRegistryService(); + await svc.rollback(skillId, v1Id, tmpAgentSkillsDir); + + // Files must be in tmpAgentSkillsDir, not any hardcoded path + const { readFileSync } = await import("node:fs"); + const content = readFileSync(path.join(tmpAgentSkillsDir, "code-review", "SKILL.md"), "utf-8"); + expect(content).toBe("# Version 1"); + }); +}); + +// --------------------------------------------------------------------------- +// assignGroup() / removeGroup() — use provided agentSkillsDir +// --------------------------------------------------------------------------- + +describe("assignGroup() — uses provided agentSkillsDir", () => { + it("Test 8: assignGroup installs skills into the provided agentSkillsDir", async () => { + const skillId = "nexus-skills/code-review"; + const versionId = `${skillId}@abc123`; + const cacheDir = path.join(tmpHome, "cache", "code-review"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "nexus-skills", versionId, cacheDir }); + + const db = await getSkillRegistryDb(); + const now = Date.now(); + const groupId = "custom/test-group"; + await db.insert((await import("../services/skill-registry-schema.js")).skillGroups).values({ + id: groupId, name: "Test Group", description: null, isBuiltin: 0, createdAt: now, updatedAt: now, + }); + await db.insert((await import("../services/skill-registry-schema.js")).skillGroupMembers).values({ + groupId, skillId, addedAt: now, + }); + + const grpSvc = skillGroupService(); + const result = await grpSvc.assignGroup(groupId, "agent-123", tmpAgentSkillsDir); + + // The skill should have been installed into our provided dir + expect(result.installed).toContain(skillId); + expect(existsSync(path.join(tmpAgentSkillsDir, "code-review", "SKILL.md"))).toBe(true); + }); +}); + +describe("removeGroup() — uses provided agentSkillsDir", () => { + it("Test 9: removeGroup removes skill files from the provided agentSkillsDir", async () => { + const skillId = "nexus-skills/code-review"; + const versionId = `${skillId}@abc123`; + const cacheDir = path.join(tmpHome, "cache", "code-review"); + await createFakeCacheDir(cacheDir); + await seedSkillWithVersion({ skillId, sourceId: "nexus-skills", versionId, cacheDir }); + + const db = await getSkillRegistryDb(); + const now = Date.now(); + const groupId = "custom/test-group"; + const agentId = "agent-xyz"; + const { skillGroups: grps, skillGroupMembers: grpMembers } = await import("../services/skill-registry-schema.js"); + await db.insert(grps).values({ + id: groupId, name: "Test Group", description: null, isBuiltin: 0, createdAt: now, updatedAt: now, + }); + await db.insert(grpMembers).values({ groupId, skillId, addedAt: now }); + + const grpSvc = skillGroupService(); + // Assign group (installs skills) + await grpSvc.assignGroup(groupId, agentId, tmpAgentSkillsDir); + expect(existsSync(path.join(tmpAgentSkillsDir, "code-review"))).toBe(true); + + // Remove group — should remove files from provided dir + await grpSvc.removeGroup(groupId, agentId, tmpAgentSkillsDir); + expect(existsSync(path.join(tmpAgentSkillsDir, "code-review"))).toBe(false); + }); +}); diff --git a/server/src/__tests__/skill-registry-install.test.ts b/server/src/__tests__/skill-registry-install.test.ts index 4a0b347e..1797bbc5 100644 --- a/server/src/__tests__/skill-registry-install.test.ts +++ b/server/src/__tests__/skill-registry-install.test.ts @@ -157,7 +157,7 @@ describe("skillRegistryService", () => { await seedSkillWithVersion({ skillId, sourceId: "schwepps-skills", versionId, cacheDir }); const before = Date.now(); - await svc.uninstall(skillId); + await svc.uninstall(skillId, tmpAgentSkillsDir); const after = Date.now(); const db = await getSkillRegistryDb(); @@ -174,7 +174,7 @@ describe("skillRegistryService", () => { await createFakeCacheDir(cacheDir); await seedSkillWithVersion({ skillId, sourceId: "schwepps-skills", versionId, cacheDir }); - await svc.uninstall(skillId); + await svc.uninstall(skillId, tmpAgentSkillsDir); // Not visible in normal list const normalList = await svc.list(); diff --git a/server/src/services/skill-registry-groups.ts b/server/src/services/skill-registry-groups.ts index ba453e58..30a71be9 100644 --- a/server/src/services/skill-registry-groups.ts +++ b/server/src/services/skill-registry-groups.ts @@ -370,19 +370,12 @@ export function skillGroupService() { for (const sid of effective) stillNeeded.add(sid); } - // Individually installed skills (not from a group) — these should be preserved - const individualRows = await db - .select() - .from(agentSkills) - .where(eq(agentSkills.agentId, agentId)); - const individualSkills = new Set(individualRows.map((r) => r.skillId)); - // Find skills that were contributed by the removed group const removedGroupSkills = await this.resolveEffectiveSkills(groupId); for (const skillId of removedGroupSkills) { - // Skip if still needed by another group or individually installed - if (stillNeeded.has(skillId) || individualSkills.has(skillId)) continue; + // Skip if still needed by another remaining group + if (stillNeeded.has(skillId)) continue; // Remove files from agent skills directory const slug = skillId.split("/").pop() ?? skillId;