CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-08-04 14:57:42 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

fix: harden skill sync review edges

Browse source
This commit is contained in:
Danny Avila 2026-06-07 22:38:17 -04:00
parent 03ae9003a7
commit bd3c23fd13
6 changed files with 392 additions and 79 deletions
Download patch file Download diff file Expand all files Collapse all files

2
api/server/services/Skills/sync.js
View file

@ -178,7 +178,7 @@ async function maybeRunGitHubSkillSyncForRequest(req) {
const baseConfig = await loadCurrentAppConfig();
return triggerOrchestrator.maybeRunForRequest({
...withBaseSkillSyncConfig(req, baseConfig),
skillSyncAllowServerCredentials: true,
skillSyncAllowServerCredentials: false,
});
}

32
api/server/services/Skills/sync.test.js
View file

@ -159,7 +159,7 @@ describe('GitHub skill sync service', () => {
expect(mockGetAppConfig).toHaveBeenCalledWith({ baseOnly: true });
});
it('starts a request-scoped sync for resolved admin skillSync config', async () => {
it('does not let user skill-list sync use server credentials from resolved config', async () => {
const skillSync = {
github: {
enabled: true,
@ -178,28 +178,6 @@ describe('GitHub skill sync service', () => {
],
},
};
mockRunnerStatus = {
enabled: true,
intervalMinutes: 60,
runOnStartup: false,
sources: [
{
provider: 'github',
sourceId: 'tenant-skills',
status: 'idle',
credentialPresent: true,
owner: 'LibreChat',
repo: 'skills',
ref: 'main',
paths: ['skills'],
syncedSkillCount: 0,
syncedFileCount: 0,
deletedSkillCount: 0,
deletedFileCount: 0,
},
],
credentials: [],
};
const service = require('./sync');
const started = await service.maybeRunGitHubSkillSyncForRequest({
@ -209,9 +187,9 @@ describe('GitHub skill sync service', () => {
const requestRunner = mockCreatedRunners[0].runner;
const requestConfig = await mockCreatedRunners[0].deps.getConfig();
expect(started).toBe(true);
expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(true);
expect(requestRunner.runOnce).toHaveBeenCalledTimes(1);
expect(started).toBe(false);
expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false);
expect(requestRunner.runOnce).not.toHaveBeenCalled();
expect(requestConfig.github.runOnStartup).toBe(false);
expect(requestConfig.github.sources[0]).toEqual(
expect.objectContaining({
@ -269,7 +247,7 @@ describe('GitHub skill sync service', () => {
});
expect(started).toBe(false);
expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(true);
expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false);
expect(mockCreatedRunners[0].runner.runOnce).not.toHaveBeenCalled();
});

8
packages/api/src/admin/skills.spec.ts
View file

@ -111,6 +111,10 @@ describe('createAdminSkillsSyncHandlers', () => {
expect.objectContaining({
credentialKey: undefined,
credentialPresent: false,
owner: undefined,
repo: undefined,
ref: undefined,
paths: undefined,
}),
],
}),
@ -154,6 +158,10 @@ describe('createAdminSkillsSyncHandlers', () => {
expect.objectContaining({
credentialKey: 'github-skills-prod',
credentialPresent: true,
owner: 'LibreChat',
repo: 'skills',
ref: 'main',
paths: ['skills'],
errorMessage: 'Missing GitHub credential "github-skills-prod"',
}),
],

9
packages/api/src/admin/skills.ts
View file

@ -109,6 +109,7 @@ function serializeSourceStatus(
status: ISkillSyncStatus & { credentialPresent?: boolean },
{ includeCredentialMetadata = true }: { includeCredentialMetadata?: boolean } = {},
): TGitHubSkillSyncSourceStatus {
const includePrivateSourceMetadata = includeCredentialMetadata;
return {
provider: status.provider,
sourceId: status.sourceId,
@ -116,10 +117,10 @@ function serializeSourceStatus(
status: status.status,
credentialKey: includeCredentialMetadata ? status.credentialKey : undefined,
credentialPresent: includeCredentialMetadata ? (status.credentialPresent ?? false) : false,
owner: status.owner,
repo: status.repo,
ref: status.ref,
paths: status.paths,
owner: includePrivateSourceMetadata ? status.owner : undefined,
repo: includePrivateSourceMetadata ? status.repo : undefined,
ref: includePrivateSourceMetadata ? status.ref : undefined,
paths: includePrivateSourceMetadata ? status.paths : undefined,
startedAt: toIso(status.startedAt),
finishedAt: toIso(status.finishedAt),
lastSuccessAt: toIso(status.lastSuccessAt),

206
packages/api/src/skills/sync/github.spec.ts
View file

@ -865,7 +865,7 @@ describe('createGitHubSkillSyncRunner', () => {
expect(deps.deleteSkill).toHaveBeenCalledWith(staleId.toString());
});
it('deletes stale name-conflicting mirrors before applying same-commit renames', async () => {
it('deletes stale name-conflicting mirrors after file sync and before same-commit renames', async () => {
const staleId = new Types.ObjectId();
const existingId = new Types.ObjectId();
const author = makeSourceAuthorId();
@ -939,11 +939,215 @@ describe('createGitHubSkillSyncRunner', () => {
update: expect.objectContaining({ name: 'renamed' }),
}),
);
expect((deps.upsertSkillFile as jest.Mock).mock.invocationCallOrder[0]).toBeLessThan(
deleteSkill.mock.invocationCallOrder[0],
);
expect(deleteSkill.mock.invocationCallOrder[0]).toBeLessThan(
updateSkill.mock.invocationCallOrder[0],
);
});
it('does not delete stale name-conflicting mirrors before another skill file sync fails', async () => {
const renamedMarkdown = '---\nname: renamed\ndescription: Renamed skill\n---\nBody';
const brokenMarkdown = '---\nname: broken\ndescription: Broken skill\n---\nBody';
const fetchFn = jest.fn(async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes('/commits/')) {
return response({ sha: 'commit-sha', commit: { tree: { sha: 'tree-sha' } } });
}
if (url.includes('/git/trees/tree-sha')) {
return response({
sha: 'tree-sha',
truncated: false,
tree: [
{
path: 'skills',
mode: '040000',
type: 'tree',
sha: 'skills-tree-sha',
url: 'https://api.github.test/tree/skills',
},
],
});
}
if (url.includes('/git/trees/skills-tree-sha')) {
return response({
sha: 'skills-tree-sha',
truncated: false,
tree: [
{
path: 'research/SKILL.md',
mode: '100644',
type: 'blob',
sha: 'research-skill-sha',
size: Buffer.byteLength(renamedMarkdown),
url: 'https://api.github.test/blob/research-skill',
},
{
path: 'research/scripts/run.sh',
mode: '100644',
type: 'blob',
sha: 'research-file-sha',
size: 7,
url: 'https://api.github.test/blob/research-file',
},
{
path: 'broken/SKILL.md',
mode: '100644',
type: 'blob',
sha: 'broken-skill-sha',
size: Buffer.byteLength(brokenMarkdown),
url: 'https://api.github.test/blob/broken-skill',
},
{
path: 'broken/scripts/run.sh',
mode: '100644',
type: 'blob',
sha: 'broken-file-sha',
size: 7,
url: 'https://api.github.test/blob/broken-file',
},
],
});
}
if (url.includes('/git/blobs/research-skill-sha')) {
return response(blob(renamedMarkdown));
}
if (url.includes('/git/blobs/broken-skill-sha')) {
return response(blob(brokenMarkdown));
}
if (url.includes('/git/blobs/research-file-sha')) {
return response(blob('echo ok'));
}
if (url.includes('/git/blobs/broken-file-sha')) {
return response(blob('echo ok'));
}
return response({ message: 'not found' }, 404);
}) as unknown as typeof fetch;
const staleId = new Types.ObjectId();
const existingId = new Types.ObjectId();
const author = makeSourceAuthorId();
const makeExisting = (
upstreamId: string,
_id: Types.ObjectId,
name: string,
): ISkill & { _id: Types.ObjectId } => {
const skill = makeSkill({
name,
description: `${name} skill`,
body: 'Old body',
author,
authorName: 'GitHub Sync',
source: 'github',
sourceMetadata: { provider: 'github', sourceId: 'librechat-skills', upstreamId },
});
skill._id = _id;
return skill;
};
const staleSkill = makeExisting('librechat-skills:skills/removed', staleId, 'renamed');
const syncedSkill = makeExisting('librechat-skills:skills/research', existingId, 'research');
const createdIds: string[] = [];
const deleteSkill = jest.fn(async (id: string) => ({ deleted: createdIds.includes(id) }));
const deps = createDeps({
fetchFn,
findSkillBySourceIdentity: jest.fn(async ({ upstreamId }) =>
upstreamId === 'librechat-skills:skills/research' ? syncedSkill : null,
),
getSkillById: jest.fn(async (id) =>
id.toString() === existingId.toString() ? syncedSkill : null,
),
listSkillsBySource: jest.fn(async () => [staleSkill, syncedSkill]),
createSkill: jest.fn(async (input: CreateSkillInput): Promise<CreateSkillResult> => {
const skill = makeSkill(input);
createdIds.push(skill._id.toString());
return { skill, warnings: [] };
}),
saveBuffer: jest.fn(async () => {
throw new Error('storage unavailable');
}),
deleteSkill,
updateSkill: jest.fn(),
});
const runner = createGitHubSkillSyncRunner(deps);
const result = await runner.runOnce();
expect(result.status).toBe('failed');
expect(deleteSkill).not.toHaveBeenCalledWith(staleId.toString());
expect(deps.updateSkill).not.toHaveBeenCalled();
expect(deps.upsertStatus).toHaveBeenLastCalledWith(
expect.objectContaining({
status: 'failed',
errorMessage: 'storage unavailable',
}),
);
});
it('restores a stale name-conflicting mirror when the rename update fails after deletion', async () => {
const staleId = new Types.ObjectId();
const existingId = new Types.ObjectId();
const author = makeSourceAuthorId();
const makeExisting = (
upstreamId: string,
_id: Types.ObjectId,
name: string,
): ISkill & { _id: Types.ObjectId } => {
const skill = makeSkill({
name,
description: `${name} skill`,
body: 'Old body',
author,
authorName: 'GitHub Sync',
source: 'github',
sourceMetadata: { provider: 'github', sourceId: 'librechat-skills', upstreamId },
});
skill._id = _id;
return skill;
};
const staleSkill = makeExisting('librechat-skills:skills/removed', staleId, 'renamed');
const syncedSkill = makeExisting('librechat-skills:skills/research', existingId, 'research');
const deletedIds = new Set<string>();
let restoredSkill: (ISkill & { _id: Types.ObjectId }) | undefined;
const createSkill = jest.fn(async (input: CreateSkillInput): Promise<CreateSkillResult> => {
restoredSkill = makeSkill(input);
return { skill: restoredSkill, warnings: [] };
});
const deleteSkill = jest.fn(async (id: string) => {
deletedIds.add(id);
return { deleted: true };
});
const deps = createDeps({
fetchFn: githubFetch('---\nname: renamed\ndescription: Renamed skill\n---\nBody'),
findSkillBySourceIdentity: jest.fn(async ({ upstreamId }) =>
upstreamId === 'librechat-skills:skills/research' ? syncedSkill : null,
),
getSkillById: jest.fn(async (id) =>
id.toString() === existingId.toString() ? syncedSkill : null,
),
listSkillsBySource: jest.fn(async () =>
[staleSkill, syncedSkill].filter((skill) => !deletedIds.has(skill._id.toString())),
),
createSkill,
deleteSkill,
updateSkill: jest.fn(async () => ({ status: 'conflict' as const, current: syncedSkill })),
});
const runner = createGitHubSkillSyncRunner(deps);
const result = await runner.runOnce();
expect(result.status).toBe('failed');
expect(deleteSkill).toHaveBeenCalledWith(staleId.toString());
expect(createSkill).toHaveBeenCalledWith(
expect.objectContaining({
name: 'renamed',
sourceMetadata: expect.objectContaining({
upstreamId: 'librechat-skills:skills/removed',
}),
}),
);
expect(deps.grantPermission).toHaveBeenCalledWith(
expect.objectContaining({ resourceId: restoredSkill?._id }),
);
});
it("does not mirror-delete another tenant's skills from an ambient source run", async () => {
const ambientStaleId = new Types.ObjectId();
const otherTenantId = new Types.ObjectId();

214
packages/api/src/skills/sync/github.ts
View file

@ -118,6 +118,11 @@ type StoredSkillFileRef = {
tenantId?: string;
};
type DeletedSyncedSkillJournal = {
skill: ISkill & { _id: Types.ObjectId };
files: Array<ISkillFile & { _id: Types.ObjectId }>;
};
type SyncSkillFilesJournal = {
staleFiles: StoredSkillFileRef[];
savedFiles: StoredSkillFileRef[];
@ -970,6 +975,36 @@ function toSkillFileInput(file: ISkillFile & { _id: Types.ObjectId }): UpsertSki
};
}
function toCreateSkillInput(skill: ISkill & { _id: Types.ObjectId }): CreateSkillInput {
return {
name: skill.name,
displayTitle: skill.displayTitle,
description: skill.description,
body: skill.body,
frontmatter: skill.frontmatter,
category: skill.category,
author: skill.author,
authorName: skill.authorName,
source: PROVIDER,
sourceMetadata: skill.sourceMetadata,
alwaysApply: skill.alwaysApply,
tenantId: skill.tenantId,
};
}
function toStoredFileRefFromSkillFile(
file: ISkillFile & { _id: Types.ObjectId },
): StoredSkillFileRef {
return {
filepath: file.filepath,
source: file.source,
storageKey: file.storageKey,
storageRegion: file.storageRegion,
author: file.author,
tenantId: file.tenantId,
};
}
function getStoredFileKey(file: StoredSkillFileRef): string {
return [file.source, file.filepath, file.storageKey ?? '', file.storageRegion ?? ''].join(':');
}
@ -1018,6 +1053,43 @@ async function restoreExistingSkillFiles(params: {
});
}
async function deleteSyncedSkillForRestore(
deps: GitHubSkillSyncDeps,
skill: ISkill & { _id: Types.ObjectId },
): Promise<{ deletedFileCount: number; deletedSkill: DeletedSyncedSkillJournal }> {
const files = await deps.listSkillFiles(skill._id);
await deps.deleteSkill(skill._id.toString());
return {
deletedFileCount: files.length,
deletedSkill: { skill, files },
};
}
async function restoreDeletedSyncedSkill(
deps: GitHubSkillSyncDeps,
deleted: DeletedSyncedSkillJournal,
): Promise<void> {
const restored = await deps.createSkill(toCreateSkillInput(deleted.skill));
for (const file of deleted.files) {
await deps.upsertSkillFile({
...toSkillFileInput(file),
skillId: restored.skill._id,
});
}
await ensurePublicViewer(deps, restored.skill._id);
}
async function cleanupDeletedSyncedSkillFiles(
deps: GitHubSkillSyncDeps,
deleted: DeletedSyncedSkillJournal,
): Promise<void> {
await cleanupStoredFiles({
deps,
files: deleted.files.map(toStoredFileRefFromSkillFile),
logMessage: '[GitHubSkillSync] Failed to clean up deleted stale mirrored skill file:',
});
}
function comparableSourceMetadata(metadata: Record<string, unknown> | undefined): string {
const { commitSha: _commitSha, syncedAt: _syncedAt, ...rest } = metadata ?? {};
return JSON.stringify(rest);
@ -1062,6 +1134,48 @@ function findMovedSourceSkill(params: {
);
}
function hasNameConflictingStaleSkill(params: {
source: SkillSyncGitHubSourceConfig;
prepared: PreparedDiscoveredSkill;
existingSyncedSkills: Array<ISkill & { _id: Types.ObjectId }>;
discoveredUpstreamIds: Set<string>;
}): boolean {
return Boolean(
findMovedSourceSkill({
source: params.source,
prepared: params.prepared.prepared,
existingSyncedSkills: params.existingSyncedSkills,
seenUpstreamIds: params.discoveredUpstreamIds,
}),
);
}
function orderPreparedSkillsForSafeStaleDeletes(params: {
source: SkillSyncGitHubSourceConfig;
preparedSkills: PreparedDiscoveredSkill[];
existingSyncedSkills: Array<ISkill & { _id: Types.ObjectId }>;
discoveredUpstreamIds: Set<string>;
}): PreparedDiscoveredSkill[] {
const regular: PreparedDiscoveredSkill[] = [];
const nameConflicting: PreparedDiscoveredSkill[] = [];
for (const prepared of params.preparedSkills) {
if (
prepared.prepared.existing &&
hasNameConflictingStaleSkill({
source: params.source,
prepared,
existingSyncedSkills: params.existingSyncedSkills,
discoveredUpstreamIds: params.discoveredUpstreamIds,
})
) {
nameConflicting.push(prepared);
continue;
}
regular.push(prepared);
}
return [...regular, ...nameConflicting];
}
function getMirrorNameKey(params: {
tenantId?: string;
author: string;
@ -1092,10 +1206,10 @@ function assertNoDuplicatePreparedSkillNames(
}
}
async function deleteNameConflictingStaleSkills(params: {
async function deleteNameConflictingStaleSkill(params: {
deps: GitHubSkillSyncDeps;
source: SkillSyncGitHubSourceConfig;
preparedSkills: PreparedDiscoveredSkill[];
prepared: PreparedRemoteSkill;
existingSyncedSkills: Array<ISkill & { _id: Types.ObjectId }>;
discoveredUpstreamIds: Set<string>;
assertNotCancelled: AssertNotCancelled;
@ -1103,20 +1217,15 @@ async function deleteNameConflictingStaleSkills(params: {
remainingSkills: Array<ISkill & { _id: Types.ObjectId }>;
deletedSkillCount: number;
deletedFileCount: number;
deletedSkill?: DeletedSyncedSkillJournal;
}> {
const sourceTenantId = params.source.tenantId ?? undefined;
const conflictingUpdateKeys = new Set(
params.preparedSkills
.filter(({ prepared }) => prepared.existing)
.map(({ prepared }) =>
getMirrorNameKey({
tenantId: sourceTenantId,
author: prepared.createInput.author.toString(),
name: prepared.update.name,
}),
),
);
if (conflictingUpdateKeys.size === 0) {
const staleSkill = findMovedSourceSkill({
source: params.source,
prepared: params.prepared,
existingSyncedSkills: params.existingSyncedSkills,
seenUpstreamIds: params.discoveredUpstreamIds,
});
if (!staleSkill) {
return {
remainingSkills: params.existingSyncedSkills,
deletedSkillCount: 0,
@ -1124,31 +1233,21 @@ async function deleteNameConflictingStaleSkills(params: {
};
}
const remainingSkills: Array<ISkill & { _id: Types.ObjectId }> = [];
let deletedSkillCount = 0;
let deletedFileCount = 0;
for (const skill of params.existingSyncedSkills) {
params.assertNotCancelled();
const upstreamId = getSourceMetadataString(skill, 'upstreamId');
const shouldDelete =
(skill.tenantId ?? undefined) === sourceTenantId &&
(!upstreamId || !params.discoveredUpstreamIds.has(upstreamId)) &&
conflictingUpdateKeys.has(
getMirrorNameKey({
tenantId: sourceTenantId,
author: skill.author.toString(),
name: skill.name,
}),
);
if (!shouldDelete) {
remainingSkills.push(skill);
continue;
}
deletedFileCount += await deleteSyncedSkill(params.deps, skill);
deletedSkillCount++;
}
params.assertNotCancelled();
const { deletedFileCount, deletedSkill } = await deleteSyncedSkillForRestore(
params.deps,
staleSkill,
);
const staleSkillId = staleSkill._id.toString();
return { remainingSkills, deletedSkillCount, deletedFileCount };
return {
remainingSkills: params.existingSyncedSkills.filter(
(skill) => skill._id.toString() !== staleSkillId,
),
deletedSkillCount: 1,
deletedFileCount,
deletedSkill,
};
}
async function syncSkillFiles(params: {
@ -1379,19 +1478,14 @@ async function syncSource(params: {
preparedSkills.map(({ discovered }) => makeUpstreamId(source, discovered.rootPath)),
);
assertNoDuplicatePreparedSkillNames(source, preparedSkills);
const staleConflictCleanup = await deleteNameConflictingStaleSkills({
deps,
const orderedPreparedSkills = orderPreparedSkillsForSafeStaleDeletes({
source,
preparedSkills,
existingSyncedSkills: await getExistingSyncedSkills(),
discoveredUpstreamIds,
assertNotCancelled,
});
existingSyncedSkills = staleConflictCleanup.remainingSkills;
counts.deletedSkillCount += staleConflictCleanup.deletedSkillCount;
counts.deletedFileCount += staleConflictCleanup.deletedFileCount;
for (const { discovered, prepared } of preparedSkills) {
for (const { discovered, prepared } of orderedPreparedSkills) {
assertNotCancelled();
const movedExisting = prepared.existing
? null
@ -1427,6 +1521,9 @@ async function syncSource(params: {
const previousFiles = await deps.listSkillFiles(effectivePrepared.existing._id);
const journal: SyncSkillFilesJournal = { staleFiles: [], savedFiles: [] };
let fileCounts: SyncSkillFilesResult;
let staleConflictCleanup:
| Awaited<ReturnType<typeof deleteNameConflictingStaleSkill>>
| undefined;
try {
fileCounts = await syncSkillFiles({
deps,
@ -1439,6 +1536,19 @@ async function syncSource(params: {
assertNotCancelled,
journal,
});
if (prepared.existing) {
staleConflictCleanup = await deleteNameConflictingStaleSkill({
deps,
source,
prepared: effectivePrepared,
existingSyncedSkills: await getExistingSyncedSkills(),
discoveredUpstreamIds,
assertNotCancelled,
});
existingSyncedSkills = staleConflictCleanup.remainingSkills;
counts.deletedSkillCount += staleConflictCleanup.deletedSkillCount;
counts.deletedFileCount += staleConflictCleanup.deletedFileCount;
}
await commitExistingRemoteSkillAfterFileSync(
deps,
{
@ -1459,6 +1569,15 @@ async function syncSource(params: {
cleanupError,
),
);
if (staleConflictCleanup?.deletedSkill) {
await restoreDeletedSyncedSkill(deps, staleConflictCleanup.deletedSkill).catch(
(cleanupError) =>
logger.error(
'[GitHubSkillSync] Failed to restore stale mirrored skill after sync failure:',
cleanupError,
),
);
}
throw error;
}
await cleanupStoredFiles({
@ -1466,6 +1585,9 @@ async function syncSource(params: {
files: fileCounts.staleFiles,
logMessage: '[GitHubSkillSync] Failed to clean up replaced synced file:',
});
if (staleConflictCleanup?.deletedSkill) {
await cleanupDeletedSyncedSkillFiles(deps, staleConflictCleanup.deletedSkill);
}
counts.syncedSkillCount++;
counts.syncedFileCount += fileCounts.syncedFileCount;
counts.deletedFileCount += fileCounts.deletedFileCount;