From 6950448d0331b49dd37050efd2e6a3d8715c3b8b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 7 Jun 2026 10:38:09 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=9B=EF=B8=8F=20refactor:=20Prioritize?= =?UTF-8?q?=20Deployment=20Skills=20over=20Persisted=20Duplicates=20(#1357?= =?UTF-8?q?5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: prefer deployment skills on name collision * chore: sort deployment skill imports * fix: dedupe deployment collision warnings * fix: return logger from warning spy * fix: preserve skill collision pagination * fix: honor db page boundary for skill merges --- .../src/skills/__tests__/deployment.test.ts | 290 +++++++++++++++++- packages/api/src/skills/deployment.ts | 150 +++++++-- 2 files changed, 409 insertions(+), 31 deletions(-) diff --git a/packages/api/src/skills/__tests__/deployment.test.ts b/packages/api/src/skills/__tests__/deployment.test.ts index 5843bd202d..ea180d240b 100644 --- a/packages/api/src/skills/__tests__/deployment.test.ts +++ b/packages/api/src/skills/__tests__/deployment.test.ts @@ -2,7 +2,9 @@ import fs from 'fs'; import os from 'os'; import path from 'path'; import { Types } from 'mongoose'; +import { logger } from '@librechat/data-schemas'; import type { CodeEnvRef } from 'librechat-data-provider'; +import type { DeploymentSkillBaseMethods } from '../deployment'; import { DEPLOYMENT_SKILLS_DIR_ENV, createDeploymentSkillMethods, @@ -12,7 +14,6 @@ import { mergeDeploymentSkillIds, resolveDeploymentSkillDirectory, } from '../deployment'; -import type { DeploymentSkillBaseMethods } from '../deployment'; const DESCRIPTION = 'Use this skill when the deployment needs a shared testing fixture.'; @@ -64,6 +65,15 @@ async function writeDeploymentSkill( return skillDir; } +function encodeTestCursor(row: { _id: Types.ObjectId; updatedAt: Date }): string { + return Buffer.from( + JSON.stringify({ + updatedAt: row.updatedAt.toISOString(), + _id: row._id.toString(), + }), + ).toString('base64'); +} + afterEach(async () => { const emptyRoot = await makeTempRoot(); await initializeDeploymentSkills({ projectRoot: emptyRoot, env: {} }); @@ -324,4 +334,282 @@ describe('createDeploymentSkillMethods', () => { (await methods.getSkillFileByPath?.(deploymentId, 'references/guide.txt'))?.codeEnvRef, ).toEqual(codeEnvRef); }); + + it('lets deployment skills shadow persisted skills with the same name', async () => { + const root = await makeTempRoot(); + await writeDeploymentSkill(root, { + skillsDir: 'config/skills', + name: 'analysis-kit', + alwaysApply: true, + }); + await initializeDeploymentSkills({ + projectRoot: root, + env: { [DEPLOYMENT_SKILLS_DIR_ENV]: 'config/skills' }, + }); + + const deploymentId = getDeploymentSkillIds()[0]; + const dbId = new Types.ObjectId(); + const otherId = new Types.ObjectId(); + const dbAuthor = new Types.ObjectId(); + const shadowedSkill = { + _id: dbId, + name: 'analysis-kit', + description: 'A persisted duplicate skill.', + body: 'persisted duplicate body', + author: dbAuthor, + version: 9, + fileCount: 0, + updatedAt: new Date(Date.now() + 60_000), + }; + const otherSkill = { + _id: otherId, + name: 'db-skill', + description: 'A persisted non-duplicate skill.', + body: 'persisted body', + author: dbAuthor, + version: 1, + fileCount: 0, + updatedAt: new Date(0), + }; + const base: DeploymentSkillBaseMethods = { + getSkillByName: jest.fn(async (name) => (name === shadowedSkill.name ? shadowedSkill : null)), + listSkillsByAccess: jest.fn(async () => ({ + skills: [shadowedSkill, otherSkill], + has_more: false, + after: null, + })), + listAlwaysApplySkills: jest.fn(async () => ({ + skills: [ + { + _id: dbId, + name: shadowedSkill.name, + body: shadowedSkill.body, + author: dbAuthor, + updatedAt: shadowedSkill.updatedAt, + }, + { + _id: otherId, + name: 'db-always', + body: 'db always', + author: dbAuthor, + updatedAt: otherSkill.updatedAt, + }, + ], + has_more: false, + after: null, + })), + }; + + const methods = createDeploymentSkillMethods(base); + const mergedIds = mergeDeploymentSkillIds([dbId, otherId]); + const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + const byName = await methods.getSkillByName?.(shadowedSkill.name, mergedIds, { + preferUserInvocable: true, + }); + expect(byName).toMatchObject({ + _id: deploymentId, + name: shadowedSkill.name, + source: 'deployment', + }); + expect(base.getSkillByName).not.toHaveBeenCalled(); + + const listed = await methods.listSkillsByAccess?.({ + accessibleIds: mergedIds, + limit: 10, + }); + expect(listed?.skills.map((skill) => [skill.name, skill._id.toString()]).sort()).toEqual([ + ['analysis-kit', deploymentId.toString()], + ['db-skill', otherId.toString()], + ]); + + await methods.listSkillsByAccess?.({ + accessibleIds: mergedIds, + limit: 10, + }); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy).toHaveBeenNthCalledWith( + 1, + expect.stringContaining('persisted list skill row(s) shadowed'), + ); + + const alwaysApply = await methods.listAlwaysApplySkills?.({ + accessibleIds: mergedIds, + limit: 10, + }); + expect(alwaysApply?.skills.map((skill) => [skill.name, skill._id.toString()]).sort()).toEqual([ + ['analysis-kit', deploymentId.toString()], + ['db-always', otherId.toString()], + ]); + + await methods.listAlwaysApplySkills?.({ + accessibleIds: mergedIds, + limit: 10, + }); + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy).toHaveBeenNthCalledWith( + 2, + expect.stringContaining('persisted always-apply skill row(s) shadowed'), + ); + warnSpy.mockRestore(); + }); + + it('preserves pagination when an always-apply DB page only contains shadowed skills', async () => { + const root = await makeTempRoot(); + await writeDeploymentSkill(root, { + skillsDir: 'config/skills', + name: 'analysis-kit', + alwaysApply: false, + }); + await initializeDeploymentSkills({ + projectRoot: root, + env: { [DEPLOYMENT_SKILLS_DIR_ENV]: 'config/skills' }, + }); + + const dbId = new Types.ObjectId(); + const nextId = new Types.ObjectId(); + const dbAuthor = new Types.ObjectId(); + const shadowedUpdatedAt = new Date('2026-01-02T00:00:00.000Z'); + const shadowedSkill = { + _id: dbId, + name: 'analysis-kit', + body: 'persisted duplicate body', + author: dbAuthor, + }; + const nextSkill = { + _id: nextId, + name: 'db-next', + body: 'next persisted body', + author: dbAuthor, + }; + const shadowedCursor = encodeTestCursor({ _id: dbId, updatedAt: shadowedUpdatedAt }); + const base: DeploymentSkillBaseMethods = { + listAlwaysApplySkills: jest.fn(async (params) => { + if (params.cursor) { + return { + skills: [nextSkill], + has_more: false, + after: null, + }; + } + return { + skills: [shadowedSkill], + has_more: true, + after: shadowedCursor, + }; + }), + }; + + const methods = createDeploymentSkillMethods(base); + const mergedIds = mergeDeploymentSkillIds([dbId, nextId]); + const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + const first = await methods.listAlwaysApplySkills?.({ + accessibleIds: mergedIds, + limit: 1, + }); + expect(first?.skills).toEqual([]); + expect(first?.has_more).toBe(true); + expect(first?.after).toBe(shadowedCursor); + + const second = await methods.listAlwaysApplySkills?.({ + accessibleIds: mergedIds, + limit: 1, + cursor: first?.after, + }); + expect(second?.skills.map((skill) => skill.name)).toEqual(['db-next']); + expect(second?.has_more).toBe(false); + expect(base.listAlwaysApplySkills).toHaveBeenNthCalledWith(2, { + accessibleIds: [dbId, nextId], + limit: 1, + cursor: first?.after, + }); + warnSpy.mockRestore(); + }); + + it('waits to return older deployment rows until the DB page boundary advances', async () => { + const root = await makeTempRoot(); + await writeDeploymentSkill(root, { + skillsDir: 'config/skills', + name: 'analysis-kit', + alwaysApply: false, + }); + await initializeDeploymentSkills({ + projectRoot: root, + env: { [DEPLOYMENT_SKILLS_DIR_ENV]: 'config/skills' }, + }); + + const hiddenId = new Types.ObjectId(); + const visibleId = new Types.ObjectId(); + const nextId = new Types.ObjectId(); + const dbAuthor = new Types.ObjectId(); + const hiddenSkill = { + _id: hiddenId, + name: 'analysis-kit', + description: 'A persisted duplicate skill.', + author: dbAuthor, + version: 1, + fileCount: 0, + updatedAt: new Date('2026-12-03T00:00:00.000Z'), + }; + const visibleSkill = { + _id: visibleId, + name: 'db-visible', + description: 'A visible persisted skill.', + author: dbAuthor, + version: 1, + fileCount: 0, + updatedAt: new Date('2026-12-02T00:00:00.000Z'), + }; + const nextSkill = { + _id: nextId, + name: 'db-next', + description: 'The next persisted skill.', + author: dbAuthor, + version: 1, + fileCount: 0, + updatedAt: new Date('2026-12-01T00:00:00.000Z'), + }; + const visibleCursor = encodeTestCursor({ + _id: visibleId, + updatedAt: visibleSkill.updatedAt, + }); + const base: DeploymentSkillBaseMethods = { + listSkillsByAccess: jest.fn(async (params) => { + if (params.cursor) { + return { + skills: [nextSkill], + has_more: false, + after: null, + }; + } + return { + skills: [hiddenSkill, visibleSkill], + has_more: true, + after: visibleCursor, + }; + }), + }; + + const methods = createDeploymentSkillMethods(base); + const mergedIds = mergeDeploymentSkillIds([hiddenId, visibleId, nextId]); + const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + const first = await methods.listSkillsByAccess?.({ + accessibleIds: mergedIds, + limit: 2, + }); + expect(first?.skills.map((skill) => skill.name)).toEqual(['db-visible']); + expect(first?.has_more).toBe(true); + expect(first?.after).toBe(visibleCursor); + + const second = await methods.listSkillsByAccess?.({ + accessibleIds: mergedIds, + limit: 2, + cursor: first?.after, + }); + expect(second?.skills.map((skill) => skill.name)).toEqual(['db-next', 'analysis-kit']); + expect(second?.has_more).toBe(false); + warnSpy.mockRestore(); + }); }); diff --git a/packages/api/src/skills/deployment.ts b/packages/api/src/skills/deployment.ts index d46cc69f42..3824fe055f 100644 --- a/packages/api/src/skills/deployment.ts +++ b/packages/api/src/skills/deployment.ts @@ -1,7 +1,7 @@ import fs from 'fs'; import path from 'path'; -import crypto from 'crypto'; import yaml from 'js-yaml'; +import crypto from 'crypto'; import { Types } from 'mongoose'; import { logger, @@ -182,6 +182,9 @@ export type DeploymentSkillBaseMethods = { }; type Cursor = { updatedAt: Date; _id: Types.ObjectId }; +type CollisionFilterResult = { + rows: T[]; +}; type LoadDeploymentSkillsOptions = { projectRoot?: string; @@ -251,6 +254,17 @@ export class DeploymentSkillRegistry { return skill; } + namesByAccess(accessibleIds: Types.ObjectId[]): Set { + const accessibleSet = new Set(accessibleIds.map((id) => id.toString())); + const names = new Set(); + for (const skill of this.list()) { + if (accessibleSet.has(skill._id.toString())) { + names.add(skill.name); + } + } + return names; + } + listByAccess(params: ListSkillsByAccessParams): DeploymentSkill[] { const accessibleSet = new Set(params.accessibleIds.map((id) => id.toString())); const cursor = decodeCursor(params.cursor); @@ -316,6 +330,7 @@ export class DeploymentSkillRegistry { } let registry = new DeploymentSkillRegistry(null, []); +const warnedDeploymentNameCollisions = new Set(); export function getDeploymentSkillRegistry(): DeploymentSkillRegistry { return registry; @@ -451,14 +466,14 @@ export function createDeploymentSkillMethods => { + const deployment = registry.getByName(name, accessibleIds, options); + if (deployment) { + return toSkillDetailRow(deployment); + } const dbSkill = base.getSkillByName ? await base.getSkillByName(name, stripDeploymentIds(accessibleIds), options) : null; - const deployment = registry.getByName(name, accessibleIds, options); - return pickPreferredSkill( - [dbSkill, deployment ? toSkillDetailRow(deployment) : null], - options, - ); + return dbSkill; }, listSkillsByAccess: async ( params: ListSkillsByAccessParams, @@ -469,9 +484,20 @@ export function createDeploymentSkillMethods boundedLimit || dbResult.has_more === true; + const cursorRow = getMergedPageCursor(sliced, boundedLimit, dbPageBoundary); return { skills: sliced, has_more: hasMore, - after: hasMore && sliced.length > 0 ? encodeCursor(sliced[sliced.length - 1]) : null, + after: hasMore && cursorRow ? encodeCursor(cursorRow) : null, }; } function mergeAlwaysApplyPage({ dbResult, + dbPageBoundary, deploymentRows, limit, }: { dbResult: ListAlwaysApplyResult; + dbPageBoundary?: Cursor | null; deploymentRows: AlwaysApplySkillRow[]; limit: number; }): ListAlwaysApplyResult { @@ -825,36 +870,49 @@ function mergeAlwaysApplyPage({ const merged = [...deploymentRows, ...dbResult.skills].sort(compareBySkillCursor); const sliced = merged.slice(0, boundedLimit); const hasMore = merged.length > boundedLimit || dbResult.has_more === true; + const cursorRow = getMergedPageCursor(sliced, boundedLimit, dbPageBoundary); return { skills: sliced, has_more: hasMore, - after: hasMore && sliced.length > 0 ? encodeCursor(sliced[sliced.length - 1]) : null, + after: hasMore && cursorRow ? encodeCursor(cursorRow) : null, }; } -function pickPreferredSkill( - rows: Array, - options?: SkillLookupOptions, -): SkillDetailRow | null { - const candidates = rows.filter((row): row is SkillDetailRow => row != null); - if (candidates.length === 0) { - return null; +function getMergedPageCursor>( + sliced: T[], + boundedLimit: number, + dbPageBoundary?: Cursor | null, +): Pick | null { + const lastReturned = sliced.length > 0 ? sliced[sliced.length - 1] : null; + if (sliced.length < boundedLimit && dbPageBoundary) { + return dbPageBoundary; } - const preferred = candidates.filter((row) => matchesLookupPreference(row, options)); - return (preferred.length > 0 ? preferred : candidates).sort(compareBySkillCursor)[0]; + return lastReturned; } -function matchesLookupPreference( - row: Pick, - options?: SkillLookupOptions, -): boolean { - if (options?.preferUserInvocable === true && row.userInvocable === false) { - return false; +function getDbPageBoundary>(dbResult: { + skills: T[]; + has_more?: boolean; + after?: string | null; +}): Cursor | null { + if (dbResult.has_more !== true) { + return null; } - if (options?.preferModelInvocable === true && row.disableModelInvocation === true) { - return false; + const last = dbResult.skills.length > 0 ? dbResult.skills[dbResult.skills.length - 1] : null; + if (last?.updatedAt) { + return { _id: last._id, updatedAt: last.updatedAt }; } - return true; + return decodeCursor(dbResult.after); +} + +function limitRowsToDbPageBoundary>( + rows: T[], + dbPageBoundary: Cursor | null, +): T[] { + if (!dbPageBoundary) { + return rows; + } + return rows.filter((row) => compareBySkillCursor(row, dbPageBoundary) <= 0); } function toSkillSummaryRow(skill: DeploymentSkill): SkillSummaryRow { @@ -902,6 +960,38 @@ function hasAccessibleId(ids: Types.ObjectId[], skillId: Types.ObjectId): boolea return ids.some((id) => id.toString() === key); } +function filterDeploymentNameCollisions< + T extends { name: string } & Pick, +>(rows: T[], deploymentNames: Set, context: string): CollisionFilterResult { + if (rows.length === 0 || deploymentNames.size === 0) { + return { rows }; + } + const hiddenNames = new Set(); + const filtered = rows.filter((row) => { + if (!deploymentNames.has(row.name)) { + return true; + } + hiddenNames.add(row.name); + return false; + }); + if (hiddenNames.size > 0) { + const unwarnedNames = Array.from(hiddenNames).filter((name) => { + const key = `${context}:${name}`; + if (warnedDeploymentNameCollisions.has(key)) { + return false; + } + warnedDeploymentNameCollisions.add(key); + return true; + }); + if (unwarnedNames.length > 0) { + logger.warn( + `[deploymentSkills] Hid persisted ${context} skill row(s) shadowed by deployment skill name(s): ${unwarnedNames.join(', ')}`, + ); + } + } + return { rows: filtered }; +} + function compareBySkillCursor( a: Pick, b: Pick,