🏛️ refactor: Prioritize Deployment Skills over Persisted Duplicates (#13575)

* 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
This commit is contained in:
Danny Avila 2026-06-07 10:38:09 -04:00 committed by GitHub
parent 15108f0f2f
commit 6950448d03
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 409 additions and 31 deletions

View file

@ -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();
});
});

View file

@ -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<T> = {
rows: T[];
};
type LoadDeploymentSkillsOptions = {
projectRoot?: string;
@ -251,6 +254,17 @@ export class DeploymentSkillRegistry {
return skill;
}
namesByAccess(accessibleIds: Types.ObjectId[]): Set<string> {
const accessibleSet = new Set(accessibleIds.map((id) => id.toString()));
const names = new Set<string>();
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<string>();
export function getDeploymentSkillRegistry(): DeploymentSkillRegistry {
return registry;
@ -451,14 +466,14 @@ export function createDeploymentSkillMethods<T extends DeploymentSkillBaseMethod
accessibleIds: Types.ObjectId[],
options?: SkillLookupOptions,
): Promise<SkillDetailRow | null> => {
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<T extends DeploymentSkillBaseMethod
accessibleIds: stripDeploymentIds(params.accessibleIds),
})
: { skills: [], has_more: false, after: null };
const deploymentNames = registry.namesByAccess(params.accessibleIds);
const filteredDb = filterDeploymentNameCollisions(dbResult.skills, deploymentNames, 'list');
const dbPageBoundary = getDbPageBoundary(dbResult);
const deploymentRows = limitRowsToDbPageBoundary(
registry.listByAccess(params).map(toSkillSummaryRow),
dbPageBoundary,
);
return mergeSkillPage({
dbResult,
deploymentRows: registry.listByAccess(params).map(toSkillSummaryRow),
dbResult: {
...dbResult,
skills: filteredDb.rows,
},
dbPageBoundary,
deploymentRows,
limit: params.limit,
});
},
@ -484,9 +510,23 @@ export function createDeploymentSkillMethods<T extends DeploymentSkillBaseMethod
accessibleIds: stripDeploymentIds(params.accessibleIds),
})
: { skills: [], has_more: false, after: null };
const deploymentNames = registry.namesByAccess(params.accessibleIds);
const filteredDb = filterDeploymentNameCollisions(
dbResult.skills,
deploymentNames,
'always-apply',
);
const dbPageBoundary = getDbPageBoundary(dbResult);
return mergeAlwaysApplyPage({
dbResult,
deploymentRows: registry.listAlwaysApply(params).map(toAlwaysApplyRow),
dbResult: {
...dbResult,
skills: filteredDb.rows,
},
dbPageBoundary,
deploymentRows: limitRowsToDbPageBoundary(
registry.listAlwaysApply(params).map(toAlwaysApplyRow),
dbPageBoundary,
),
limit: params.limit,
});
},
@ -794,10 +834,12 @@ function validateUniqueNames(skills: DeploymentSkill[]): void {
function mergeSkillPage({
dbResult,
dbPageBoundary,
deploymentRows,
limit,
}: {
dbResult: ListSkillsByAccessResult;
dbPageBoundary?: Cursor | null;
deploymentRows: SkillSummaryRow[];
limit: number;
}): ListSkillsByAccessResult {
@ -805,19 +847,22 @@ function mergeSkillPage({
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 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<SkillDetailRow | null>,
options?: SkillLookupOptions,
): SkillDetailRow | null {
const candidates = rows.filter((row): row is SkillDetailRow => row != null);
if (candidates.length === 0) {
return null;
function getMergedPageCursor<T extends Pick<SkillSummaryRow, '_id' | 'updatedAt'>>(
sliced: T[],
boundedLimit: number,
dbPageBoundary?: Cursor | null,
): Pick<SkillSummaryRow, '_id' | 'updatedAt'> | 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<SkillSummaryRow, 'disableModelInvocation' | 'userInvocable'>,
options?: SkillLookupOptions,
): boolean {
if (options?.preferUserInvocable === true && row.userInvocable === false) {
return false;
function getDbPageBoundary<T extends Pick<SkillSummaryRow, '_id' | 'updatedAt'>>(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<T extends Pick<SkillSummaryRow, '_id' | 'updatedAt'>>(
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<SkillSummaryRow, '_id' | 'updatedAt'>,
>(rows: T[], deploymentNames: Set<string>, context: string): CollisionFilterResult<T> {
if (rows.length === 0 || deploymentNames.size === 0) {
return { rows };
}
const hiddenNames = new Set<string>();
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<SkillSummaryRow, '_id' | 'updatedAt'>,
b: Pick<SkillSummaryRow, '_id' | 'updatedAt'>,