diff --git a/changelog.d/fixes/10854-skills-marketplace-owner.md b/changelog.d/fixes/10854-skills-marketplace-owner.md new file mode 100644 index 0000000000..e80a109f77 --- /dev/null +++ b/changelog.d/fixes/10854-skills-marketplace-owner.md @@ -0,0 +1 @@ +- **fix(skills):** Marketplace-installed skills are available to API-key-scoped requests, including existing SkillsMP and skills.sh installs ([#10854](https://github.com/diegosouzapw/OmniRoute/pull/10854)) — thanks @kriptoburak diff --git a/src/app/api/skills/install/route.ts b/src/app/api/skills/install/route.ts index 540ee02e56..d881a0f7dd 100644 --- a/src/app/api/skills/install/route.ts +++ b/src/app/api/skills/install/route.ts @@ -1,6 +1,6 @@ import { NextResponse } from "next/server"; import { z } from "zod"; -import { skillRegistry } from "@/lib/skills/registry"; +import { GLOBAL_SKILL_OWNER_ID, skillRegistry } from "@/lib/skills/registry"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; @@ -39,7 +39,7 @@ export async function POST(request: Request) { description, schema: { input: schema.input, output: schema.output }, handler: handlerCode, - apiKeyId: apiKeyId || "system", + apiKeyId: apiKeyId || GLOBAL_SKILL_OWNER_ID, enabled: true, }); diff --git a/src/app/api/skills/marketplace/install/route.ts b/src/app/api/skills/marketplace/install/route.ts index 1690e2b686..7480ca5992 100644 --- a/src/app/api/skills/marketplace/install/route.ts +++ b/src/app/api/skills/marketplace/install/route.ts @@ -1,7 +1,7 @@ import { NextResponse } from "next/server"; import { z } from "zod"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; -import { skillRegistry } from "@/lib/skills/registry"; +import { GLOBAL_SKILL_OWNER_ID, skillRegistry } from "@/lib/skills/registry"; import { getSkillsProviderSetting } from "@/lib/skills/providerSettings"; import { isAuthenticated } from "@/shared/utils/apiAuth"; @@ -44,7 +44,7 @@ export async function POST(request: Request) { description, schema: { input: { content: "string" }, output: { result: "string" } }, handler: `// Installed from SkillsMP\n// SKILL.md content:\n${skillMdContent}`, - apiKeyId: provider, + apiKeyId: GLOBAL_SKILL_OWNER_ID, enabled: true, mode: "auto", sourceProvider: "skillsmp", diff --git a/src/app/api/skills/skillssh/install/route.ts b/src/app/api/skills/skillssh/install/route.ts index 84707c0e73..ec74c64a0c 100644 --- a/src/app/api/skills/skillssh/install/route.ts +++ b/src/app/api/skills/skillssh/install/route.ts @@ -1,7 +1,7 @@ import { NextResponse } from "next/server"; import { z } from "zod"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; -import { skillRegistry } from "@/lib/skills/registry"; +import { GLOBAL_SKILL_OWNER_ID, skillRegistry } from "@/lib/skills/registry"; import { isAuthenticated } from "@/shared/utils/apiAuth"; import { fetchSkillMd } from "@/lib/skills/skillssh"; import { getSkillsProviderSetting } from "@/lib/skills/providerSettings"; @@ -46,7 +46,7 @@ export async function POST(request: Request) { description, schema: { input: { content: "string" }, output: { result: "string" } }, handler: `// Installed from skills.sh\n// Source: ${source}/${skillId}\n// SKILL.md content:\n${skillMdContent}`, - apiKeyId: provider, + apiKeyId: GLOBAL_SKILL_OWNER_ID, enabled: true, mode: "auto", sourceProvider: "skillssh", diff --git a/src/lib/skills/registry.ts b/src/lib/skills/registry.ts index fd39e65554..4621ff9a6d 100644 --- a/src/lib/skills/registry.ts +++ b/src/lib/skills/registry.ts @@ -1,4 +1,4 @@ -import { Skill, SkillSchema } from "./types"; +import type { Skill, SkillSchema } from "./types"; import { SkillCreateInputSchema } from "./schemas"; import { getDbInstance } from "../db/core"; import { randomUUID } from "crypto"; @@ -6,6 +6,10 @@ import { logger } from "../../../open-sse/utils/logger.ts"; const log = logger("SKILLS"); +export const GLOBAL_SKILL_OWNER_ID = "system"; +const GLOBAL_SKILL_OWNER_IDS = [GLOBAL_SKILL_OWNER_ID, "skillsmp", "skillssh"] as const; +const GLOBAL_SKILL_OWNER_ID_SET = new Set(GLOBAL_SKILL_OWNER_IDS); + class SkillRegistry { private static instance: SkillRegistry; private registeredSkills: Map = new Map(); @@ -39,6 +43,36 @@ class SkillRegistry { return `${skill.apiKeyId}:${skill.name}@${skill.version}`; } + private skillIdentity(skill: Pick): string { + return `${skill.name}@${skill.version}`; + } + + private isGlobalOwner(apiKeyId: string): boolean { + return GLOBAL_SKILL_OWNER_ID_SET.has(apiKeyId); + } + + private scopedSkills(apiKeyId?: string): Skill[] { + const skills = Array.from(this.registeredSkills.values()); + if (!apiKeyId) return skills; + + const owned = skills.filter((skill) => skill.apiKeyId === apiKeyId); + const visibleIdentities = new Set(owned.map((skill) => this.skillIdentity(skill))); + const global = [ + ...skills.filter((skill) => skill.apiKeyId === GLOBAL_SKILL_OWNER_ID), + ...skills.filter( + (skill) => skill.apiKeyId !== GLOBAL_SKILL_OWNER_ID && this.isGlobalOwner(skill.apiKeyId) + ), + ]; + for (const skill of global) { + const identity = this.skillIdentity(skill); + if (!visibleIdentities.has(identity)) { + owned.push(skill); + visibleIdentities.add(identity); + } + } + return owned; + } + private cacheSkill(skill: Skill): void { this.registeredSkills.set(this.cacheKey(skill), skill); this.updateVersionCache(skill); @@ -180,16 +214,11 @@ class SkillRegistry { list(apiKeyId?: string): Skill[] { log.debug("skills.registry.list", { apiKeyId, cached: !this.isCacheStale() }); - if (apiKeyId) { - return Array.from(this.registeredSkills.values()).filter((s) => s.apiKeyId === apiKeyId); - } - return Array.from(this.registeredSkills.values()); + return this.scopedSkills(apiKeyId); } getSkill(identifier: string, apiKeyId?: string): Skill | undefined { - const matchesScope = (skill: Skill) => !apiKeyId || skill.apiKeyId === apiKeyId; - const skills = Array.from(this.registeredSkills.values()).filter(matchesScope); - + const skills = this.scopedSkills(apiKeyId); const byId = skills.find((skill) => skill.id === identifier); if (byId) return byId; @@ -206,8 +235,8 @@ class SkillRegistry { } getSkillVersions(name: string, apiKeyId?: string): Skill[] { - return Array.from(this.registeredSkills.values()) - .filter((skill) => skill.name === name && (!apiKeyId || skill.apiKeyId === apiKeyId)) + return this.scopedSkills(apiKeyId) + .filter((skill) => skill.name === name) .sort((a, b) => this.compareVersions(b.version, a.version)); } @@ -304,12 +333,23 @@ class SkillRegistry { try { log.debug("skills.registry.loadFromDatabase", { cached: false }); const db = getDbInstance(); - const rows = apiKeyId - ? db.prepare("SELECT * FROM skills WHERE api_key_id = ?").all(apiKeyId) - : db.prepare("SELECT * FROM skills").all(); + let rows: unknown[]; + if (!apiKeyId) { + rows = db.prepare("SELECT * FROM skills").all(); + } else if (this.isGlobalOwner(apiKeyId)) { + rows = db + .prepare("SELECT * FROM skills WHERE api_key_id IN (?, ?, ?)") + .all(...GLOBAL_SKILL_OWNER_IDS); + } else { + rows = db + .prepare("SELECT * FROM skills WHERE api_key_id IN (?, ?, ?, ?)") + .all(apiKeyId, ...GLOBAL_SKILL_OWNER_IDS); + } if (apiKeyId) { - this.removeCachedSkills((skill) => skill.apiKeyId === apiKeyId); + this.removeCachedSkills( + (skill) => skill.apiKeyId === apiKeyId || this.isGlobalOwner(skill.apiKeyId) + ); } else { this.registeredSkills.clear(); this.versionCache.clear(); diff --git a/tests/unit/skills-injection.test.ts b/tests/unit/skills-injection.test.ts index d9b941331d..efbca1823c 100644 --- a/tests/unit/skills-injection.test.ts +++ b/tests/unit/skills-injection.test.ts @@ -8,7 +8,7 @@ const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-skills-in process.env.DATA_DIR = TEST_DATA_DIR; const coreDb = await import("../../src/lib/db/core.ts"); -const { skillRegistry } = await import("../../src/lib/skills/registry.ts"); +const { GLOBAL_SKILL_OWNER_ID, skillRegistry } = await import("../../src/lib/skills/registry.ts"); const { injectSkills, injectSkillTools, detectProvider, decodeSkillToolName } = await import("../../src/lib/skills/injection.ts"); @@ -103,6 +103,35 @@ test("injectSkills renders enabled tools in provider-specific shapes", async () assert.deepEqual(fallbackTools, [openaiTools[0]]); }); +test("injectSkills includes global skills without leaking another API key's skills", async () => { + await skillRegistry.register({ + name: "releaseNotes", + version: "1.0.0", + description: "draft release notes", + schema: { input: {}, output: {} }, + handler: "release-notes-handler", + enabled: true, + apiKeyId: GLOBAL_SKILL_OWNER_ID, + }); + await skillRegistry.register({ + name: "privateSkill", + version: "1.0.0", + description: "private skill", + schema: { input: {}, output: {} }, + handler: "private-handler", + enabled: true, + apiKeyId: "key-b", + }); + + const tools = injectSkills({ provider: "openai", apiKeyId: "key-a" }); + + assert.equal(tools.length, 1); + assert.equal( + decodeSkillToolName((tools[0] as { function: { name: string } }).function.name), + "releaseNotes@1.0.0" + ); +}); + test("injectSkillTools only injects into the last user message without tools", async () => { await registerSkills(); diff --git a/tests/unit/skills-marketplace.test.ts b/tests/unit/skills-marketplace.test.ts new file mode 100644 index 0000000000..64fddb7cd9 --- /dev/null +++ b/tests/unit/skills-marketplace.test.ts @@ -0,0 +1,67 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-skills-marketplace-")); +const originalDataDir = process.env.DATA_DIR; +process.env.DATA_DIR = tmpDir; + +const core = await import("../../src/lib/db/core.ts"); +const settingsDb = await import("../../src/lib/db/settings.ts"); +const { GLOBAL_SKILL_OWNER_ID, skillRegistry } = await import("../../src/lib/skills/registry.ts"); +const installRoute = await import("../../src/app/api/skills/marketplace/install/route.ts"); + +function clearSkillRegistry() { + skillRegistry.registeredSkills?.clear?.(); + skillRegistry.versionCache?.clear?.(); + skillRegistry.loadedApiKeyIds?.clear?.(); + skillRegistry.invalidateCache(); +} + +function resetStorage() { + core.resetDbInstance(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.mkdirSync(tmpDir, { recursive: true }); + clearSkillRegistry(); + core.getDbInstance(); +} + +test.beforeEach(async () => { + resetStorage(); + await settingsDb.updateSettings({ + skillsProvider: "skillsmp", + requireLogin: false, + }); +}); + +test.after(() => { + core.resetDbInstance(); + clearSkillRegistry(); + process.env.DATA_DIR = originalDataDir; + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +test("SkillsMP installs are available to API-key-scoped requests", async () => { + const req = new Request("http://localhost/api/skills/marketplace/install", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "release-notes", + description: "Draft release notes from repository changes", + skillMdContent: "# Release Notes\nSummarize user-facing changes.", + version: "1.0.0", + }), + }); + + const res = await installRoute.POST(req); + const body = (await res.json()) as { success: boolean; id: string }; + const installed = skillRegistry.getSkill(body.id); + + assert.equal(res.status, 200); + assert.equal(body.success, true); + assert.equal(installed?.apiKeyId, GLOBAL_SKILL_OWNER_ID); + assert.equal(installed?.sourceProvider, "skillsmp"); + assert.equal(skillRegistry.list("customer-key")[0]?.id, body.id); +}); diff --git a/tests/unit/skills-registry.test.ts b/tests/unit/skills-registry.test.ts index e0aa4afa4d..14e6a9c6a5 100644 --- a/tests/unit/skills-registry.test.ts +++ b/tests/unit/skills-registry.test.ts @@ -8,7 +8,7 @@ const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-skills-re process.env.DATA_DIR = TEST_DATA_DIR; const coreDb = await import("../../src/lib/db/core.ts"); -const { skillRegistry } = await import("../../src/lib/skills/registry.ts"); +const { GLOBAL_SKILL_OWNER_ID, skillRegistry } = await import("../../src/lib/skills/registry.ts"); function resetRegistryState() { skillRegistry["registeredSkills"].clear(); @@ -100,6 +100,69 @@ test("skillRegistry keeps same name/version isolated per API key", async () => { ); }); +test("skillRegistry exposes global skills without crossing API-key scopes", async () => { + const global = await skillRegistry.register({ + name: "marketplace-skill", + version: "1.0.0", + description: "global version", + schema: { input: {}, output: {} }, + handler: "global-handler", + apiKeyId: GLOBAL_SKILL_OWNER_ID, + }); + const globalOnly = await skillRegistry.register({ + name: "global-only", + version: "1.0.0", + description: "global skill", + schema: { input: {}, output: {} }, + handler: "global-only-handler", + apiKeyId: GLOBAL_SKILL_OWNER_ID, + }); + const legacyOnly = await skillRegistry.register({ + name: "legacy-marketplace-skill", + version: "1.0.0", + description: "legacy SkillsMP install", + schema: { input: {}, output: {} }, + handler: "legacy-handler", + apiKeyId: "skillsmp", + }); + await skillRegistry.register({ + name: "global-only", + version: "1.0.0", + description: "legacy duplicate", + schema: { input: {}, output: {} }, + handler: "legacy-duplicate-handler", + apiKeyId: "skillssh", + }); + const keyOverride = await skillRegistry.register({ + name: "marketplace-skill", + version: "1.0.0", + description: "key override", + schema: { input: {}, output: {} }, + handler: "key-handler", + apiKeyId: "key-a", + }); + await skillRegistry.register({ + name: "private-skill", + version: "1.0.0", + description: "key b only", + schema: { input: {}, output: {} }, + handler: "private-handler", + apiKeyId: "key-b", + }); + + resetRegistryState(); + await skillRegistry.loadFromDatabase("key-a"); + + assert.deepEqual( + skillRegistry.list("key-a").map((skill) => skill.id), + [keyOverride.id, globalOnly.id, legacyOnly.id] + ); + assert.equal(skillRegistry.getSkill("marketplace-skill", "key-a")?.id, keyOverride.id); + assert.equal(skillRegistry.getSkill(global.id, "key-a"), undefined); + assert.equal(skillRegistry.getSkill(globalOnly.id, "key-a")?.id, globalOnly.id); + assert.equal(skillRegistry.getSkill("private-skill", "key-a"), undefined); +}); + test("skillRegistry can reload persisted skills from SQLite", async () => { const first = await skillRegistry.register({ name: "file-read", diff --git a/tests/unit/skills-skillssh.test.ts b/tests/unit/skills-skillssh.test.ts index bf9c2cc549..c5229e9b79 100644 --- a/tests/unit/skills-skillssh.test.ts +++ b/tests/unit/skills-skillssh.test.ts @@ -10,7 +10,7 @@ process.env.DATA_DIR = tmpDir; const core = await import("../../src/lib/db/core.ts"); const settingsDb = await import("../../src/lib/db/settings.ts"); -const { skillRegistry } = await import("../../src/lib/skills/registry.ts"); +const { GLOBAL_SKILL_OWNER_ID, skillRegistry } = await import("../../src/lib/skills/registry.ts"); const { searchSkillsSh, fetchSkillMd, SkillsShSearchResponseSchema, SkillsShSkillSchema } = await import("../../src/lib/skills/skillssh.ts"); const searchRoute = await import("../../src/app/api/skills/skillssh/route.ts"); @@ -238,7 +238,8 @@ test("skillssh install route registers a skill from skills.sh", async () => { const skills = skillRegistry.list(); const installed = skills.find((s) => s.name === "docker-best-practices"); assert.ok(installed); - assert.equal(installed.apiKeyId, "skillssh"); + assert.equal(installed.apiKeyId, GLOBAL_SKILL_OWNER_ID); + assert.equal(skillRegistry.list("customer-key")[0]?.id, installed.id); assert.ok(installed.handler.includes("Installed from skills.sh")); assert.ok(installed.handler.includes(mdContent)); });