diff --git a/packages/skills-catalog/package.json b/packages/skills-catalog/package.json index 8a6f80ac..46b46a2e 100644 --- a/packages/skills-catalog/package.json +++ b/packages/skills-catalog/package.json @@ -14,6 +14,7 @@ "type": "module", "exports": { ".": "./src/index.ts", + "./package.json": "./package.json", "./types": "./src/types.ts", "./catalog.json": "./generated/catalog.json" }, @@ -24,6 +25,7 @@ "types": "./dist/src/index.d.ts", "import": "./dist/src/index.js" }, + "./package.json": "./package.json", "./types": { "types": "./dist/src/types.d.ts", "import": "./dist/src/types.js" diff --git a/server/package.json b/server/package.json index 0f897a84..0dc249de 100644 --- a/server/package.json +++ b/server/package.json @@ -58,6 +58,7 @@ "@paperclipai/db": "workspace:*", "@paperclipai/plugin-sdk": "workspace:*", "@paperclipai/shared": "workspace:*", + "@paperclipai/skills-catalog": "workspace:*", "ajv": "^8.20.0", "ajv-formats": "^3.0.1", "better-auth": "1.4.18", diff --git a/server/src/__tests__/company-skills-routes.test.ts b/server/src/__tests__/company-skills-routes.test.ts index acd567a0..5436b2f4 100644 --- a/server/src/__tests__/company-skills-routes.test.ts +++ b/server/src/__tests__/company-skills-routes.test.ts @@ -31,7 +31,7 @@ const mockCompanySkillService = vi.hoisted(() => ({ })); const mockCatalogService = vi.hoisted(() => ({ - listCatalogSkills: vi.fn(), + listCatalogSkillsOrEmpty: vi.fn(), getCatalogSkillOrThrow: vi.fn(), readCatalogSkillFile: vi.fn(), })); @@ -115,6 +115,7 @@ describe("company skill mutation permissions", () => { imported: [], warnings: [], }); + mockCatalogService.listCatalogSkillsOrEmpty.mockReturnValue([]); mockCompanySkillService.list.mockResolvedValue([]); mockCompanySkillService.categoryCounts.mockResolvedValue([]); mockCompanySkillService.detail.mockResolvedValue(null); @@ -260,7 +261,7 @@ describe("company skill mutation permissions", () => { slug: "find-skills", name: "Find Skills", }); - mockCatalogService.listCatalogSkills.mockReturnValue([]); + mockCatalogService.listCatalogSkillsOrEmpty.mockReturnValue([]); mockCatalogService.getCatalogSkillOrThrow.mockReturnValue({ id: "paperclipai:bundled:software-development:review", key: "paperclipai/bundled/software-development/review", @@ -312,7 +313,7 @@ describe("company skill mutation permissions", () => { }); it("serves catalog listing without mutating company skills", async () => { - mockCatalogService.listCatalogSkills.mockReturnValue([ + mockCatalogService.listCatalogSkillsOrEmpty.mockReturnValue([ { id: "paperclipai:bundled:software-development:review", key: "paperclipai/bundled/software-development/review", @@ -344,7 +345,7 @@ describe("company skill mutation permissions", () => { .get("/api/skills/catalog?kind=bundled&q=review"); expect(res.status, JSON.stringify(res.body)).toBe(200); - expect(mockCatalogService.listCatalogSkills).toHaveBeenCalledWith({ kind: "bundled", q: "review" }); + expect(mockCatalogService.listCatalogSkillsOrEmpty).toHaveBeenCalledWith({ kind: "bundled", q: "review" }); expect(mockCompanySkillService.importFromSource).not.toHaveBeenCalled(); expect(mockCompanySkillService.installFromCatalog).not.toHaveBeenCalled(); expect(mockLogActivity).not.toHaveBeenCalled(); @@ -360,7 +361,7 @@ describe("company skill mutation permissions", () => { expect(list.status, JSON.stringify(list.body)).toBe(401); expect(detail.status, JSON.stringify(detail.body)).toBe(401); expect(file.status, JSON.stringify(file.body)).toBe(401); - expect(mockCatalogService.listCatalogSkills).not.toHaveBeenCalled(); + expect(mockCatalogService.listCatalogSkillsOrEmpty).not.toHaveBeenCalled(); expect(mockCatalogService.getCatalogSkillOrThrow).not.toHaveBeenCalled(); expect(mockCatalogService.readCatalogSkillFile).not.toHaveBeenCalled(); }); diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index 177e2072..7fbdf008 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -1030,8 +1030,15 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { }) ); expect(issue?.executionRunId).toBe(retryRun?.id ?? null); + + const checkoutReleasedIssue = await waitForValue(async () => + db.select().from(issues).where(eq(issues.id, issueId)).then((rows) => { + const row = rows[0] ?? null; + return row?.checkoutRunId === null ? row : null; + }) + ); // Terminal run cleanup releases the checkout lock so future checkout 409s only mean a live owner exists. - expect(issue?.checkoutRunId).toBeNull(); + expect(checkoutReleasedIssue?.checkoutRunId).toBeNull(); }); it("releases active environment leases when an orphaned run is reaped", async () => { @@ -1519,8 +1526,11 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { }); expect(recoveryAction?.nextAction).toContain("Bind the missing secret"); - const comments = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); - expect(comments.some((comment) => comment.body.includes("secret/env bindings are missing"))).toBe(true); + const configurationComment = await waitForValue(async () => { + const rows = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); + return rows.find((comment) => comment.body.includes("secret/env bindings are missing")) ?? null; + }); + expect(configurationComment).toBeTruthy(); }); it("queues one finish-handoff wake when a successful run leaves in-progress work without a next action", async () => { diff --git a/server/src/__tests__/skills-catalog-service.test.ts b/server/src/__tests__/skills-catalog-service.test.ts index 44fed3cc..51ab751d 100644 --- a/server/src/__tests__/skills-catalog-service.test.ts +++ b/server/src/__tests__/skills-catalog-service.test.ts @@ -6,6 +6,8 @@ const mockExistsSync = vi.hoisted(() => vi.fn()); const mockReadFileSync = vi.hoisted(() => vi.fn()); const mockStatSync = vi.hoisted(() => vi.fn()); const mockReadFile = vi.hoisted(() => vi.fn()); +const mockRequireResolve = vi.hoisted(() => vi.fn()); +const mockLoggerWarn = vi.hoisted(() => vi.fn()); vi.doMock("node:fs", async () => { const actual = await vi.importActual("node:fs"); @@ -21,6 +23,22 @@ vi.doMock("node:fs", async () => { }; }); +vi.doMock("node:module", async () => { + const actual = await vi.importActual("node:module"); + return { + ...actual, + createRequire: () => ({ + resolve: mockRequireResolve, + }), + }; +}); + +vi.doMock("../middleware/logger.js", () => ({ + logger: { + warn: mockLoggerWarn, + }, +})); + function catalogSkill(slug: string, name = slug): CatalogSkill { return { id: `paperclipai:bundled:software-development:${slug}`, @@ -74,6 +92,15 @@ describe("skills catalog service", () => { size: Buffer.byteLength(manifestJson), })); mockReadFile.mockImplementation(async (filePath: string) => `content:${filePath}`); + mockRequireResolve.mockImplementation((specifier: string) => { + if (specifier === "@paperclipai/skills-catalog/package.json") { + return "/published/node_modules/@paperclipai/skills-catalog/package.json"; + } + if (specifier === "@paperclipai/skills-catalog/catalog.json") { + return "/published/node_modules/@paperclipai/skills-catalog/generated/catalog.json"; + } + throw new Error(`Unexpected specifier: ${specifier}`); + }); }); it("caches and reloads the generated catalog manifest when it changes", async () => { @@ -150,4 +177,59 @@ describe("skills catalog service", () => { expect(fetchMock).toHaveBeenCalledTimes(1); expect(mockReadFile).not.toHaveBeenCalled(); }); + + it("resolves the manifest and bundled skill files from the published package layout", async () => { + const publishedMarkdown = "---\nname: published\n---\n\n# Published\n"; + const publishedSkill = catalogSkill("published-skill", "Published Skill"); + publishedSkill.files = [ + { + path: "SKILL.md", + kind: "skill", + sizeBytes: Buffer.byteLength(publishedMarkdown), + sha256: sha256(publishedMarkdown), + }, + ]; + manifestJson = manifest([publishedSkill], "0.3.2"); + mockReadFile.mockImplementationOnce(async () => Buffer.from(publishedMarkdown)); + const service = await import("../services/skills-catalog.js"); + + expect(service.getCatalogPackageMetadata()).toEqual({ + packageName: "@paperclipai/skills-catalog", + packageVersion: "0.3.2", + }); + await expect(service.readCatalogSkillFile(publishedSkill.id, "SKILL.md")).resolves.toMatchObject({ + catalogSkillId: publishedSkill.id, + path: "SKILL.md", + content: publishedMarkdown, + markdown: true, + }); + expect(mockReadFileSync).toHaveBeenCalledWith( + "/published/node_modules/@paperclipai/skills-catalog/generated/catalog.json", + "utf8", + ); + expect(mockReadFile).toHaveBeenCalledWith( + "/published/node_modules/@paperclipai/skills-catalog/catalog/bundled/software-development/published-skill/SKILL.md", + ); + expect(mockRequireResolve).toHaveBeenCalledWith("@paperclipai/skills-catalog/package.json"); + expect(mockRequireResolve).toHaveBeenCalledWith("@paperclipai/skills-catalog/catalog.json"); + }); + + it("returns an empty list, caches package-resolution failure, and logs only once when the manifest cannot be resolved", async () => { + mockRequireResolve.mockImplementation(() => { + const error = new Error("not found") as Error & { code?: string }; + error.code = "ERR_MODULE_NOT_FOUND"; + throw error; + }); + mockExistsSync.mockReturnValue(false); + const service = await import("../services/skills-catalog.js"); + + expect(service.listCatalogSkillsOrEmpty()).toEqual([]); + expect(service.listCatalogSkillsOrEmpty()).toEqual([]); + expect(mockRequireResolve).toHaveBeenCalledTimes(1); + expect(mockLoggerWarn).toHaveBeenCalledWith( + { err: expect.any(service.CatalogManifestUnavailableError) }, + "skills catalog manifest unavailable; returning empty catalog", + ); + expect(mockLoggerWarn).toHaveBeenCalledTimes(1); + }); }); diff --git a/server/src/routes/company-skills.ts b/server/src/routes/company-skills.ts index c6bec7a6..fe7556f2 100644 --- a/server/src/routes/company-skills.ts +++ b/server/src/routes/company-skills.ts @@ -19,7 +19,11 @@ import { import { trackSkillImported } from "@paperclipai/shared/telemetry"; import { validate } from "../middleware/validate.js"; import { accessService, agentService, companySkillService, logActivity } from "../services/index.js"; -import { getCatalogSkillOrThrow, listCatalogSkills, readCatalogSkillFile } from "../services/skills-catalog.js"; +import { + getCatalogSkillOrThrow, + listCatalogSkillsOrEmpty, + readCatalogSkillFile, +} from "../services/skills-catalog.js"; import { forbidden } from "../errors.js"; import { assertAuthenticated, assertCompanyAccess, getActorInfo } from "./authz.js"; import { getTelemetryClient } from "../telemetry.js"; @@ -121,7 +125,7 @@ export function companySkillRoutes(db: Db) { category: firstQueryString(req.query.category), q: firstQueryString(req.query.q), }); - res.json(listCatalogSkills(query)); + res.json(listCatalogSkillsOrEmpty(query)); }); router.get("/skills/catalog/:catalogId/files", async (req, res) => { diff --git a/server/src/services/skills-catalog.ts b/server/src/services/skills-catalog.ts index d8ce8f9c..ffe6a798 100644 --- a/server/src/services/skills-catalog.ts +++ b/server/src/services/skills-catalog.ts @@ -1,6 +1,7 @@ import { createHash } from "node:crypto"; import { existsSync, readFileSync, statSync } from "node:fs"; import { promises as fs } from "node:fs"; +import { createRequire } from "node:module"; import path from "node:path"; import { fileURLToPath } from "node:url"; import type { @@ -10,6 +11,7 @@ import type { CatalogSkillSource, } from "@paperclipai/shared"; import { HttpError, conflict, notFound, unprocessable } from "../errors.js"; +import { logger } from "../middleware/logger.js"; import { ghFetch, resolveRawGitHubUrl } from "./github-fetch.js"; import { normalizePortablePath } from "./portable-path.js"; @@ -21,30 +23,122 @@ interface CatalogManifestFile { const serviceDir = path.dirname(fileURLToPath(import.meta.url)); const repoRoot = path.resolve(serviceDir, "../../.."); -const catalogPackageRoot = path.join(repoRoot, "packages/skills-catalog"); -const catalogManifestPath = path.join(catalogPackageRoot, "generated/catalog.json"); +const require = createRequire(import.meta.url); +const catalogPackageName = "@paperclipai/skills-catalog"; +const catalogPackageJsonSpecifier = `${catalogPackageName}/package.json`; +const catalogManifestSpecifier = `${catalogPackageName}/catalog.json`; +const devCatalogPackageRoot = path.join(repoRoot, "packages/skills-catalog"); +const devCatalogManifestPath = path.join(devCatalogPackageRoot, "generated/catalog.json"); let cachedCatalogManifest: { manifest: CatalogManifestFile; mtimeMs: number; size: number; } | null = null; +let cachedCatalogPaths: + | { + packageRoot: string; + manifestPath: string; +} + | false + | null = null; +let cachedCatalogPathsError: CatalogManifestUnavailableError | null = null; +let loggedCatalogUnavailableWarning = false; -function loadCatalogManifest(): CatalogManifestFile { - if (!existsSync(catalogManifestPath)) { - throw new Error( - `Skills catalog manifest not found at ${catalogManifestPath}. Run pnpm --filter @paperclipai/skills-catalog build:manifest.`, - ); +export class CatalogManifestUnavailableError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message); + this.name = "CatalogManifestUnavailableError"; + if (options && "cause" in options) { + Object.defineProperty(this, "cause", { + value: options.cause, + enumerable: false, + configurable: true, + writable: true, + }); + } + } +} + +export function isCatalogManifestUnavailableError(error: unknown): error is CatalogManifestUnavailableError { + return error instanceof CatalogManifestUnavailableError; +} + +function manifestUnavailableMessage(manifestPath: string) { + return `Skills catalog manifest not found at ${manifestPath}. Run pnpm --filter @paperclipai/skills-catalog build:manifest.`; +} + +function packageResolutionFailureMessage() { + return `Skills catalog package could not be resolved from ${catalogPackageJsonSpecifier} and ${catalogManifestSpecifier}.`; +} + +function isMissingFileError(error: unknown): error is NodeJS.ErrnoException { + return error instanceof Error && "code" in error && (error as NodeJS.ErrnoException).code === "ENOENT"; +} + +function resolvePublishedCatalogPaths() { + return { + packageRoot: path.dirname(require.resolve(catalogPackageJsonSpecifier)), + manifestPath: require.resolve(catalogManifestSpecifier), + }; +} + +function resolveDevCatalogPaths() { + if (!existsSync(devCatalogManifestPath)) return null; + return { + packageRoot: devCatalogPackageRoot, + manifestPath: devCatalogManifestPath, + }; +} + +function resolveCatalogPaths() { + if (cachedCatalogPaths === false && cachedCatalogPathsError) { + throw cachedCatalogPathsError; + } + if (cachedCatalogPaths) { + return cachedCatalogPaths; + } + try { + cachedCatalogPaths = resolvePublishedCatalogPaths(); + cachedCatalogPathsError = null; + return cachedCatalogPaths; + } catch (publishedError) { + const devPaths = resolveDevCatalogPaths(); + if (devPaths) { + cachedCatalogPaths = devPaths; + cachedCatalogPathsError = null; + return cachedCatalogPaths; + } + cachedCatalogPathsError = new CatalogManifestUnavailableError(packageResolutionFailureMessage(), { cause: publishedError }); + cachedCatalogPaths = false; + throw cachedCatalogPathsError; + } +} + +function loadCatalogManifest(manifestPath: string): CatalogManifestFile { + try { + return JSON.parse(readFileSync(manifestPath, "utf8")) as CatalogManifestFile; + } catch (error) { + if (isMissingFileError(error)) { + throw new CatalogManifestUnavailableError(manifestUnavailableMessage(manifestPath), { cause: error }); + } + throw error; } - return JSON.parse(readFileSync(catalogManifestPath, "utf8")) as CatalogManifestFile; } function getCatalogManifest() { - if (!existsSync(catalogManifestPath)) { - throw new Error( - `Skills catalog manifest not found at ${catalogManifestPath}. Run pnpm --filter @paperclipai/skills-catalog build:manifest.`, - ); + const { manifestPath } = resolveCatalogPaths(); + if (!existsSync(manifestPath)) { + throw new CatalogManifestUnavailableError(manifestUnavailableMessage(manifestPath)); + } + let stats: ReturnType; + try { + stats = statSync(manifestPath); + } catch (error) { + if (isMissingFileError(error)) { + throw new CatalogManifestUnavailableError(manifestUnavailableMessage(manifestPath), { cause: error }); + } + throw error; } - const stats = statSync(catalogManifestPath); if ( cachedCatalogManifest && cachedCatalogManifest.mtimeMs === stats.mtimeMs && @@ -53,7 +147,7 @@ function getCatalogManifest() { return cachedCatalogManifest.manifest; } - const manifest = loadCatalogManifest(); + const manifest = loadCatalogManifest(manifestPath); cachedCatalogManifest = { manifest, mtimeMs: stats.mtimeMs, @@ -93,7 +187,7 @@ function inferLanguageFromPath(filePath: string) { } function resolveCatalogPackageRoot() { - return catalogPackageRoot; + return resolveCatalogPaths().packageRoot; } function sourceRootPath(source: CatalogSkillSource) { @@ -173,6 +267,23 @@ export function listCatalogSkills(query: CatalogSkillListQuery = {}): CatalogSki .sort((left, right) => left.name.localeCompare(right.name) || left.key.localeCompare(right.key)); } +export function listCatalogSkillsOrEmpty(query: CatalogSkillListQuery = {}): CatalogSkill[] { + try { + const skills = listCatalogSkills(query); + loggedCatalogUnavailableWarning = false; + return skills; + } catch (error) { + if (!isCatalogManifestUnavailableError(error)) { + throw error; + } + if (!loggedCatalogUnavailableWarning) { + logger.warn({ err: error }, "skills catalog manifest unavailable; returning empty catalog"); + loggedCatalogUnavailableWarning = true; + } + return []; + } +} + export function resolveCatalogSkillReference(reference: string): { skill: CatalogSkill | null; ambiguous: boolean } { const trimmed = reference.trim(); if (!trimmed) return { skill: null, ambiguous: false };