diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index 407d10d5c4..f19005639f 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -178,7 +178,7 @@ async function maybeRunGitHubSkillSyncForRequest(req) { const baseConfig = await loadCurrentAppConfig(); return triggerOrchestrator.maybeRunForRequest({ ...withBaseSkillSyncConfig(req, baseConfig), - skillSyncAllowServerCredentials: true, + skillSyncAllowServerCredentials: false, }); } diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index 7722372808..069ffc6db2 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -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(); }); diff --git a/packages/api/src/admin/skills.spec.ts b/packages/api/src/admin/skills.spec.ts index 4453babbfb..43312c872e 100644 --- a/packages/api/src/admin/skills.spec.ts +++ b/packages/api/src/admin/skills.spec.ts @@ -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"', }), ], diff --git a/packages/api/src/admin/skills.ts b/packages/api/src/admin/skills.ts index d6d6db3291..3105f08b67 100644 --- a/packages/api/src/admin/skills.ts +++ b/packages/api/src/admin/skills.ts @@ -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), diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index a5a04583f0..406ddb5c9e 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -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 => { + 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(); + let restoredSkill: (ISkill & { _id: Types.ObjectId }) | undefined; + const createSkill = jest.fn(async (input: CreateSkillInput): Promise => { + 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(); diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 72ebbf6c52..505b6bb88e 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -118,6 +118,11 @@ type StoredSkillFileRef = { tenantId?: string; }; +type DeletedSyncedSkillJournal = { + skill: ISkill & { _id: Types.ObjectId }; + files: Array; +}; + 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 { + 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 { + await cleanupStoredFiles({ + deps, + files: deleted.files.map(toStoredFileRefFromSkillFile), + logMessage: '[GitHubSkillSync] Failed to clean up deleted stale mirrored skill file:', + }); +} + function comparableSourceMetadata(metadata: Record | 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; + discoveredUpstreamIds: Set; +}): 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; + discoveredUpstreamIds: Set; +}): 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; discoveredUpstreamIds: Set; assertNotCancelled: AssertNotCancelled; @@ -1103,20 +1217,15 @@ async function deleteNameConflictingStaleSkills(params: { remainingSkills: Array; 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 = []; - 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> + | 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;