mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-21 14:42:20 +03:00
fix(skills): make marketplace installs available to API keys (#10854)
Dashboard-installed SkillsMP and skills.sh skills now store under the canonical global skill scope and merge into every API-key-scoped lookup, so marketplace installs actually reach API keys instead of staying invisible outside the installing session. Tenant-owned skill overrides stay isolated; existing skillsmp/skillssh rows are recognized without a migration, with canonical rows preferred on identity overlap. Closes #9716. Validated in an isolated worktree boarded onto origin/release/v3.8.50 (0 conflicts, 9 files): - 94/94 skills-*.test.ts tests pass, including the tenant-isolation regression coverage in skills-injection.test.ts (global skills reach a different API key without leaking another tenant's skills). - check-file-size, check-changelog-integrity: OK. - typecheck:core: clean. - check-complexity / check-cognitive-complexity: OK, both under baseline. Co-authored-by: kriptoburak <kriptoburak@users.noreply.github.com>
This commit is contained in:
1
changelog.d/fixes/10854-skills-marketplace-owner.md
Normal file
1
changelog.d/fixes/10854-skills-marketplace-owner.md
Normal file
@@ -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
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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<string>(GLOBAL_SKILL_OWNER_IDS);
|
||||
|
||||
class SkillRegistry {
|
||||
private static instance: SkillRegistry;
|
||||
private registeredSkills: Map<string, Skill> = new Map();
|
||||
@@ -39,6 +43,36 @@ class SkillRegistry {
|
||||
return `${skill.apiKeyId}:${skill.name}@${skill.version}`;
|
||||
}
|
||||
|
||||
private skillIdentity(skill: Pick<Skill, "name" | "version">): 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();
|
||||
|
||||
@@ -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();
|
||||
|
||||
|
||||
67
tests/unit/skills-marketplace.test.ts
Normal file
67
tests/unit/skills-marketplace.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
@@ -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",
|
||||
|
||||
@@ -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));
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user