From 7c7e27a6b869e925b40ffcba034a4186d60fe1b9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 30 May 2026 12:16:41 -0700 Subject: [PATCH] fix: Harden GitHub skill sync review paths --- api/server/experimental.js | 6 + api/server/routes/admin/skills.js | 7 +- api/server/routes/admin/skills.test.js | 16 +- packages/api/src/skills/sync/github.spec.ts | 189 +++++++++++++++++ packages/api/src/skills/sync/github.ts | 224 ++++++++++++++++---- 5 files changed, 387 insertions(+), 55 deletions(-) diff --git a/api/server/experimental.js b/api/server/experimental.js index 8fd94c6723..bd19bec072 100644 --- a/api/server/experimental.js +++ b/api/server/experimental.js @@ -23,8 +23,10 @@ const { } = require('@librechat/api'); const { connectDb, indexSync } = require('~/db'); const initializeOAuthReconnectManager = require('./services/initializeOAuthReconnectManager'); +const { capabilityContextMiddleware } = require('./middleware/roles/capabilities'); const createValidateImageRequest = require('./middleware/validateImageRequest'); const { startExpiredFileSweep } = require('./services/Files/process'); +const { initializeGitHubSkillSync } = require('./services/Skills/sync'); const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies'); const { updateInterfacePermissions: updateInterfacePerms } = require('@librechat/api'); const { @@ -294,6 +296,7 @@ if (cluster.isMaster) { /** Initialize app configuration */ const appConfig = await getAppConfig(); initializeFileStorage(appConfig); + initializeGitHubSkillSync(appConfig); expiredFileSweepOptions = { appConfig, loadAppConfig: getAppConfig }; startExpiredFileSweepOnce(); await performStartupChecks(appConfig); @@ -370,10 +373,13 @@ if (cluster.isMaster) { await configureSocialLogins(app); } + app.use(capabilityContextMiddleware); + /** Routes */ app.use('/oauth', routes.oauth); app.use('/api/auth', routes.auth); app.use('/api/admin', routes.adminAuth); + app.use('/api/admin/skills', routes.adminSkills); app.use('/api/actions', routes.actions); app.use('/api/keys', routes.keys); app.use('/api/api-keys', routes.apiKeys); diff --git a/api/server/routes/admin/skills.js b/api/server/routes/admin/skills.js index 47f86928b4..820b2e44c0 100644 --- a/api/server/routes/admin/skills.js +++ b/api/server/routes/admin/skills.js @@ -8,6 +8,7 @@ const { getGitHubSkillSyncRunner } = require('~/server/services/Skills/sync'); const router = express.Router(); const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN); +const requireManageSkills = requireCapability(SystemCapabilities.MANAGE_SKILLS); const handlers = createAdminSkillsSyncHandlers({ runner: getGitHubSkillSyncRunner(), @@ -18,8 +19,8 @@ const handlers = createAdminSkillsSyncHandlers({ router.use(requireJwtAuth, requireAdminAccess); router.get('/sync/status', handlers.getSyncStatus); -router.post('/sync/run', handlers.runSync); -router.put('/sync/credentials/:credentialKey', handlers.setCredential); -router.delete('/sync/credentials/:credentialKey', handlers.deleteCredential); +router.post('/sync/run', requireManageSkills, handlers.runSync); +router.put('/sync/credentials/:credentialKey', requireManageSkills, handlers.setCredential); +router.delete('/sync/credentials/:credentialKey', requireManageSkills, handlers.deleteCredential); module.exports = router; diff --git a/api/server/routes/admin/skills.test.js b/api/server/routes/admin/skills.test.js index 4378ef0b5e..07471238b6 100644 --- a/api/server/routes/admin/skills.test.js +++ b/api/server/routes/admin/skills.test.js @@ -2,11 +2,11 @@ const express = require('express'); const request = require('supertest'); const mockRequireJwtAuth = jest.fn((req, res, next) => next()); -const mockRequireAdminAccess = jest.fn((req, res, next) => next()); -const mockRequireCapability = jest.fn(() => mockRequireAdminAccess); +const mockCapabilityMiddleware = jest.fn((req, res, next) => next()); +const mockRequireCapability = jest.fn(() => mockCapabilityMiddleware); jest.mock('@librechat/data-schemas', () => ({ - SystemCapabilities: { ACCESS_ADMIN: 'ACCESS_ADMIN' }, + SystemCapabilities: { ACCESS_ADMIN: 'access:admin', MANAGE_SKILLS: 'manage:skills' }, })); jest.mock('@librechat/api', () => ({ @@ -39,16 +39,20 @@ jest.mock('~/server/services/Skills/sync', () => ({ })); describe('admin skills sync routes', () => { - it('requires JWT auth and ACCESS_ADMIN for sync endpoints', async () => { + it('requires JWT auth and admin capabilities for sync endpoints', async () => { const router = require('./skills'); const app = express(); app.use(express.json()); app.use('/api/admin/skills', router); await request(app).get('/api/admin/skills/sync/status').expect(200); + await request(app).post('/api/admin/skills/sync/run').expect(200); + await request(app).put('/api/admin/skills/sync/credentials/default').send({}).expect(200); + await request(app).delete('/api/admin/skills/sync/credentials/default').expect(200); - expect(mockRequireCapability).toHaveBeenCalledWith('ACCESS_ADMIN'); + expect(mockRequireCapability).toHaveBeenCalledWith('access:admin'); + expect(mockRequireCapability).toHaveBeenCalledWith('manage:skills'); expect(mockRequireJwtAuth).toHaveBeenCalled(); - expect(mockRequireAdminAccess).toHaveBeenCalled(); + expect(mockCapabilityMiddleware).toHaveBeenCalled(); }); }); diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 174267b6b5..5457644a28 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -109,6 +109,35 @@ function makeSkill(input: CreateSkillInput): ISkill & { _id: Types.ObjectId } { }; } +function makeSkillFile( + skill: ISkill & { _id: Types.ObjectId }, + overrides: Partial = {}, +): ISkillFile & { _id: Types.ObjectId } { + return { + _id: new Types.ObjectId(), + skillId: skill._id, + relativePath: 'scripts/run.sh', + file_id: 'old-file-id', + filename: 'run.sh', + filepath: '/uploads/old-file-id__run.sh', + source: 'local', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/research', + commitSha: 'old-commit-sha', + blobSha: 'old-file-sha', + path: 'skills/research/scripts/run.sh', + }, + mimeType: 'application/x-sh', + bytes: 7, + category: 'script', + isExecutable: false, + author: skill.author, + ...overrides, + }; +} + function createDeps( overrides: Partial = {}, ): GitHubSkillSyncDeps & { statuses: ISkillSyncStatus[] } { @@ -657,6 +686,146 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('skips existing skill updates when the upstream package is unchanged', async () => { + const skillMarkdown = '---\nname: research\ndescription: Research things\n---\nBody'; + const existing = makeSkill({ + name: 'research', + description: 'Research things', + body: skillMarkdown, + frontmatter: {}, + author: new Types.ObjectId(), + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/research', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/research', + commitSha: 'old-commit-sha', + skillBlobSha: 'skill-md-sha', + syncedAt: '2026-05-30T00:00:00.000Z', + syncStatus: 'synced', + }, + }) as ISkill & { _id: Types.ObjectId }; + const unchangedFile = makeSkillFile(existing, { + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/research', + commitSha: 'old-commit-sha', + blobSha: 'file-sha', + path: 'skills/research/scripts/run.sh', + }, + }); + const deps = createDeps({ + fetchFn: githubFetch(skillMarkdown), + findSkillBySourceIdentity: jest.fn(async () => existing), + getSkillById: jest.fn(async () => existing), + getSkillFileByPath: jest.fn(async () => unchangedFile), + listSkillFiles: jest.fn(async () => [unchangedFile]), + updateSkill: jest.fn(), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.saveBuffer).not.toHaveBeenCalled(); + expect(deps.upsertSkillFile).not.toHaveBeenCalled(); + expect(deps.updateSkill).not.toHaveBeenCalled(); + expect(deps.grantPermission).toHaveBeenCalled(); + }); + + it('restores existing skill files when the skill update fails after file sync', async () => { + const existing = makeSkill({ + name: 'research', + description: 'Old description', + body: 'Old body', + author: new Types.ObjectId(), + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/research', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/research', + }, + }) as ISkill & { _id: Types.ObjectId }; + const oldFile = makeSkillFile(existing); + const files = new Map([ + [oldFile.relativePath, oldFile], + ]); + const upsertSkillFile = jest.fn( + async ( + row: Parameters[0], + ): Promise => { + const current = files.get(row.relativePath); + const next = { + _id: current?._id ?? new Types.ObjectId(), + skillId: row.skillId as Types.ObjectId, + relativePath: row.relativePath, + file_id: row.file_id, + filename: row.filename, + filepath: row.filepath, + storageKey: row.storageKey, + storageRegion: row.storageRegion, + source: row.source, + sourceMetadata: row.sourceMetadata, + mimeType: row.mimeType, + bytes: row.bytes, + category: 'script' as const, + isExecutable: row.isExecutable ?? false, + author: row.author, + tenantId: row.tenantId, + }; + files.set(row.relativePath, next); + return next; + }, + ); + const deps = createDeps({ + findSkillBySourceIdentity: jest.fn(async () => existing), + getSkillById: jest.fn(async () => ({ ...existing, version: existing.version + 1 })), + getSkillFileByPath: jest.fn( + async (_skillId, relativePath) => files.get(relativePath) ?? null, + ), + listSkillFiles: jest.fn(async () => Array.from(files.values())), + upsertSkillFile, + deleteSkillFile: jest.fn(async (_skillId, relativePath) => ({ + deleted: files.delete(relativePath), + })), + saveBuffer: jest.fn(async () => ({ + filepath: '/uploads/new-file-id__run.sh', + source: 'local', + })), + deleteFile: jest.fn(async () => undefined), + updateSkill: jest.fn(async () => ({ status: 'conflict' as const })), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SKILL_CONFLICT', + }), + ); + expect(files.get('scripts/run.sh')).toEqual( + expect.objectContaining({ filepath: oldFile.filepath }), + ); + expect(deps.deleteFile).toHaveBeenCalledWith( + expect.objectContaining({ filepath: '/uploads/new-file-id__run.sh' }), + ); + expect(deps.deleteFile).not.toHaveBeenCalledWith( + expect.objectContaining({ filepath: oldFile.filepath }), + ); + }); + it('preserves credential presence when a manual run is skipped by an active lock', async () => { const deps = createDeps({ tryAcquireLock: jest.fn(async () => false), @@ -698,6 +867,26 @@ describe('createGitHubSkillSyncRunner', () => { ]); }); + it('uses a fresh lock owner for each sync run', async () => { + const deps = createDeps({ lockOwner: 'worker-a' }); + const runner = createGitHubSkillSyncRunner(deps); + + await runner.runOnce(); + await runner.runOnce(); + + const lockOwners = (deps.tryAcquireLock as jest.Mock).mock.calls.map( + ([params]: [Parameters[0]]) => params.lockOwner, + ); + const releasedOwners = (deps.releaseLock as jest.Mock).mock.calls.map( + ([params]: [Parameters[0]]) => params.lockOwner, + ); + + expect(lockOwners).toHaveLength(2); + expect(lockOwners[0]).not.toBe(lockOwners[1]); + expect(lockOwners.every((owner) => owner.startsWith('worker-a:'))).toBe(true); + expect(releasedOwners).toEqual(lockOwners); + }); + it('excludes child skill packages from parent synced files', async () => { const parentSkillMarkdown = '---\nname: parent\ndescription: Parent skill\n---\nParent'; const childSkillMarkdown = '---\nname: child\ndescription: Child skill\n---\nChild'; diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index b962a34563..526b9c1cad 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -99,6 +99,23 @@ type SaveBufferResult = { storageRegion?: string; }; +type StoredSkillFileRef = { + filepath: string; + source: string; + storageKey?: string; + storageRegion?: string; + author?: Types.ObjectId | string; + tenantId?: string; +}; + +type SyncSkillFilesJournal = { + staleFiles: StoredSkillFileRef[]; + savedFiles: StoredSkillFileRef[]; +}; + +type SyncSkillFilesResult = Pick & + SyncSkillFilesJournal; + type MaybePromise = T | Promise; export type GitHubSkillSyncDeps = { @@ -828,6 +845,7 @@ function hasExternalSkillEdit(before: ISkill, after: ISkill): boolean { async function commitExistingRemoteSkillAfterFileSync( deps: GitHubSkillSyncDeps, prepared: PreparedExistingRemoteSkill, + options: { forceCommit?: boolean } = {}, ): Promise { const refreshed = await deps.getSkillById(prepared.existing._id); if (!refreshed) { @@ -842,13 +860,13 @@ async function commitExistingRemoteSkillAfterFileSync( `Skill "${prepared.existing.name}" was modified during sync`, ); } + if (!options.forceCommit && !hasRemoteSkillDefinitionChanged(prepared.update, refreshed)) { + return { skill: refreshed, created: false }; + } return commitRemoteSkill(deps, { ...prepared, existing: refreshed }); } -async function cleanupFile( - deps: GitHubSkillSyncDeps, - file: ISkillFile & { _id: Types.ObjectId }, -): Promise { +async function cleanupFile(deps: GitHubSkillSyncDeps, file: StoredSkillFileRef): Promise { if (!deps.deleteFile) { return; } @@ -862,6 +880,105 @@ async function cleanupFile( }); } +function toStoredFileRef(params: { + saved: SaveBufferResult; + author: Types.ObjectId; + tenantId?: string; +}): StoredSkillFileRef { + return { + filepath: params.saved.filepath, + source: params.saved.source, + storageKey: params.saved.storageKey, + storageRegion: params.saved.storageRegion, + author: params.author, + tenantId: params.tenantId, + }; +} + +function toSkillFileInput(file: ISkillFile & { _id: Types.ObjectId }): UpsertSkillFileInput { + return { + skillId: file.skillId, + relativePath: file.relativePath, + file_id: file.file_id, + filename: file.filename, + filepath: file.filepath, + storageKey: file.storageKey, + storageRegion: file.storageRegion, + source: file.source, + sourceMetadata: file.sourceMetadata, + mimeType: file.mimeType, + bytes: file.bytes, + isExecutable: file.isExecutable, + author: file.author, + tenantId: file.tenantId, + }; +} + +function getStoredFileKey(file: StoredSkillFileRef): string { + return [file.source, file.filepath, file.storageKey ?? '', file.storageRegion ?? ''].join(':'); +} + +async function cleanupStoredFiles(params: { + deps: GitHubSkillSyncDeps; + files: StoredSkillFileRef[]; + logMessage: string; +}): Promise { + const seen = new Set(); + for (const file of params.files) { + const key = getStoredFileKey(file); + if (seen.has(key)) { + continue; + } + seen.add(key); + await cleanupFile(params.deps, file).catch((cleanupError) => + logger.error(params.logMessage, cleanupError), + ); + } +} + +async function restoreExistingSkillFiles(params: { + deps: GitHubSkillSyncDeps; + skill: ISkill & { _id: Types.ObjectId }; + previousFiles: Array; + savedFiles: StoredSkillFileRef[]; +}): Promise { + const { deps, skill, previousFiles, savedFiles } = params; + const previousByPath = new Map(previousFiles.map((file) => [file.relativePath, file])); + const currentFiles = await deps.listSkillFiles(skill._id); + + for (const file of currentFiles) { + if (previousByPath.has(file.relativePath)) { + continue; + } + await deps.deleteSkillFile(skill._id, file.relativePath); + } + for (const file of previousFiles) { + await deps.upsertSkillFile(toSkillFileInput(file)); + } + await cleanupStoredFiles({ + deps, + files: savedFiles, + logMessage: '[GitHubSkillSync] Failed to clean up rolled-back synced file:', + }); +} + +function comparableSourceMetadata(metadata: Record | undefined): string { + const { commitSha: _commitSha, syncedAt: _syncedAt, ...rest } = metadata ?? {}; + return JSON.stringify(rest); +} + +function hasRemoteSkillDefinitionChanged(update: UpdateSkillInput, existing: ISkill): boolean { + return ( + update.body !== existing.body || + update.name !== existing.name || + update.description !== existing.description || + (update.alwaysApply ?? false) !== (existing.alwaysApply ?? false) || + JSON.stringify(update.frontmatter ?? {}) !== JSON.stringify(existing.frontmatter ?? {}) || + comparableSourceMetadata(update.sourceMetadata) !== + comparableSourceMetadata(existing.sourceMetadata) + ); +} + async function syncSkillFiles(params: { deps: GitHubSkillSyncDeps; token: string; @@ -871,8 +988,10 @@ async function syncSkillFiles(params: { commitSha: string; fetchFn: FetchFn; assertNotCancelled: AssertNotCancelled; -}): Promise> { + journal?: SyncSkillFilesJournal; +}): Promise { const { deps, token, source, skill, discovered, commitSha, fetchFn, assertNotCancelled } = params; + const journal = params.journal ?? { staleFiles: [], savedFiles: [] }; const remotePaths = new Set(); let syncedFileCount = 0; let deletedFileCount = 0; @@ -887,7 +1006,6 @@ async function syncSkillFiles(params: { totalFileBytes += assertGitHubBlobSize(entry, relativePath); assertCumulativeGitHubFileSize(totalFileBytes); remotePaths.add(relativePath); - syncedFileCount++; const existing = await deps.getSkillFileByPath(skill._id, relativePath); if (existing && getSourceMetadataString(existing, 'blobSha') === entry.sha) { continue; @@ -906,6 +1024,7 @@ async function syncSkillFiles(params: { isImage: mimeType.startsWith('image/'), tenantId: skill.tenantId, }); + const savedFile = toStoredFileRef({ saved, author: skill.author, tenantId: skill.tenantId }); try { await deps.upsertSkillFile({ skillId: skill._id, @@ -931,29 +1050,15 @@ async function syncSkillFiles(params: { tenantId: skill.tenantId, }); } catch (error) { - if (deps.deleteFile) { - await deps - .deleteFile({ - filepath: saved.filepath, - source: saved.source, - storageKey: saved.storageKey, - storageRegion: saved.storageRegion, - user: skill.author, - tenantId: skill.tenantId, - }) - .catch((cleanupError) => - logger.error( - '[GitHubSkillSync] Failed to clean up orphaned synced file:', - cleanupError, - ), - ); - } + await cleanupFile(deps, savedFile).catch((cleanupError) => + logger.error('[GitHubSkillSync] Failed to clean up orphaned synced file:', cleanupError), + ); throw error; } + syncedFileCount++; + journal.savedFiles.push(savedFile); if (existing && existing.filepath !== saved.filepath) { - await cleanupFile(deps, existing).catch((cleanupError) => - logger.error('[GitHubSkillSync] Failed to clean up replaced synced file:', cleanupError), - ); + journal.staleFiles.push(existing); } } @@ -963,15 +1068,13 @@ async function syncSkillFiles(params: { if (remotePaths.has(file.relativePath)) { continue; } - await cleanupFile(deps, file).catch((cleanupError) => - logger.error('[GitHubSkillSync] Failed to clean up deleted synced file:', cleanupError), - ); const result = await deps.deleteSkillFile(skill._id, file.relativePath); if (result.deleted) { deletedFileCount++; + journal.staleFiles.push(file); } } - return { syncedFileCount, deletedFileCount }; + return { syncedFileCount, deletedFileCount, ...journal }; } async function deleteSyncedSkill( @@ -1068,21 +1171,50 @@ async function syncSource(params: { `Skill "${prepared.existing.name}" was modified during sync`, ); } - const fileCounts = await syncSkillFiles({ - deps, - token, - source, - skill: prepared.existing, - discovered, - commitSha: commit.sha, - fetchFn, - assertNotCancelled, - }); - const upserted = await commitExistingRemoteSkillAfterFileSync(deps, { - ...prepared, - existing: prepared.existing, - }); + const previousFiles = await deps.listSkillFiles(prepared.existing._id); + const journal: SyncSkillFilesJournal = { staleFiles: [], savedFiles: [] }; + let fileCounts: SyncSkillFilesResult; + let upserted: UpsertRemoteSkillResult; + try { + fileCounts = await syncSkillFiles({ + deps, + token, + source, + skill: prepared.existing, + discovered, + commitSha: commit.sha, + fetchFn, + assertNotCancelled, + journal, + }); + upserted = await commitExistingRemoteSkillAfterFileSync( + deps, + { + ...prepared, + existing: prepared.existing, + }, + { forceCommit: fileCounts.syncedFileCount > 0 || fileCounts.deletedFileCount > 0 }, + ); + } catch (error) { + await restoreExistingSkillFiles({ + deps, + skill: prepared.existing, + previousFiles, + savedFiles: journal.savedFiles, + }).catch((cleanupError) => + logger.error( + '[GitHubSkillSync] Failed to restore existing skill files after sync failure:', + cleanupError, + ), + ); + throw error; + } await ensurePublicViewer(deps, upserted.skill._id); + await cleanupStoredFiles({ + deps, + files: fileCounts.staleFiles, + logMessage: '[GitHubSkillSync] Failed to clean up replaced synced file:', + }); counts.syncedSkillCount++; counts.syncedFileCount += fileCounts.syncedFileCount; counts.deletedFileCount += fileCounts.deletedFileCount; @@ -1206,8 +1338,7 @@ function getGithubConfig(config: SkillSyncConfig | undefined): { export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { const fetchFn = deps.fetchFn ?? fetch; - const lockOwner = - deps.lockOwner ?? `${process.pid}:${crypto.randomUUID().replace(/-/g, '').slice(0, 12)}`; + const lockOwnerPrefix = deps.lockOwner ?? `${process.pid}`; async function getStatus() { const github = getGithubConfig(await deps.getConfig()); @@ -1261,6 +1392,7 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { if (!github.enabled || github.sources.length === 0) { return { status: 'skipped', message: 'GitHub skill sync is disabled', sources: [] }; } + const lockOwner = `${lockOwnerPrefix}:${crypto.randomUUID().replace(/-/g, '').slice(0, 12)}`; const acquired = await deps.tryAcquireLock({ provider: PROVIDER, lockOwner,