From d7f8dba1776110f988fff2b375fdd78e02a1f730 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 1 Jun 2026 19:51:22 -0400 Subject: [PATCH] feat: Trigger skill sync from resolved config --- api/server/routes/skills.js | 11 +- api/server/routes/skills.test.js | 12 ++ api/server/services/Skills/sync.js | 148 +++++++++++++-- api/server/services/Skills/sync.test.js | 176 +++++++++++++++++- packages/api/src/admin/config.handler.spec.ts | 17 +- packages/data-provider/src/config.ts | 2 +- .../data-schemas/src/app/resolution.spec.ts | 13 +- 7 files changed, 355 insertions(+), 24 deletions(-) diff --git a/api/server/routes/skills.js b/api/server/routes/skills.js index 694009a480..4537505db8 100644 --- a/api/server/routes/skills.js +++ b/api/server/routes/skills.js @@ -40,6 +40,7 @@ const { } = require('~/server/services/PermissionService'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { createFileLimiters } = require('~/server/middleware/limiters/uploadLimiters'); +const { maybeRunGitHubSkillSyncForRequest } = require('~/server/services/Skills/sync'); const configMiddleware = require('~/server/middleware/config/app'); const { getFileStrategy } = require('~/server/utils/getFileStrategy'); @@ -272,6 +273,14 @@ async function uploadFileHandler(req, res) { // --------------------------------------------------------------------------- // Routes // --------------------------------------------------------------------------- +async function maybeStartRequestSkillSync(req, _res, next) { + try { + await maybeRunGitHubSkillSyncForRequest(req); + } catch (error) { + logger.error('[GET /skills] Failed to start request-scoped skill sync:', error); + } + next(); +} // Import: accepts .md / .zip / .skill via multipart router.post( @@ -284,7 +293,7 @@ router.post( importHandler, ); -router.get('/', handlers.list); +router.get('/', maybeStartRequestSkillSync, handlers.list); router.post('/', checkSkillCreate, handlers.create); router.get( diff --git a/api/server/routes/skills.test.js b/api/server/routes/skills.test.js index af99e4bc5e..c48c0ff70b 100644 --- a/api/server/routes/skills.test.js +++ b/api/server/routes/skills.test.js @@ -33,6 +33,7 @@ const { } = require('librechat-data-provider'); let mockFileConfig; +const mockMaybeRunGitHubSkillSyncForRequest = jest.fn(async () => false); jest.mock('~/server/services/Config', () => ({ getCachedTools: jest.fn().mockResolvedValue({}), @@ -68,6 +69,10 @@ jest.mock('~/server/utils/getFileStrategy', () => ({ getFileStrategy: jest.fn().mockReturnValue('local'), })); +jest.mock('~/server/services/Skills/sync', () => ({ + maybeRunGitHubSkillSyncForRequest: mockMaybeRunGitHubSkillSyncForRequest, +})); + jest.mock('~/models', () => { const mongoose = require('mongoose'); const { createMethods } = require('@librechat/data-schemas'); @@ -152,6 +157,7 @@ afterEach(async () => { await AclEntry.deleteMany({}); currentTestUser = testUsers.owner; mockFileConfig = undefined; + mockMaybeRunGitHubSkillSyncForRequest.mockClear(); }); afterAll(async () => { @@ -409,6 +415,12 @@ describe('Skill routes', () => { setTestUser(testUsers.owner); const res = await request(app).get('/api/skills'); expect(res.status).toBe(200); + expect(mockMaybeRunGitHubSkillSyncForRequest).toHaveBeenCalledWith( + expect.objectContaining({ + config: expect.objectContaining({ fileStrategy: 'local' }), + user: expect.objectContaining({ id: testUsers.owner._id.toString() }), + }), + ); expect(res.body.skills.length).toBe(1); expect(res.body.skills[0].name).toBe('mine-skill'); }); diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index 1f806c2109..57d1cbefa2 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -1,20 +1,22 @@ -const { FileContext } = require('librechat-data-provider'); +const { FileContext, skillSyncConfigSchema } = require('librechat-data-provider'); const { getStorageMetadata, createGitHubSkillSyncRunner, startGitHubSkillSyncScheduler, } = require('@librechat/api'); -const { runAsSystem } = require('@librechat/data-schemas'); +const { logger, runAsSystem } = require('@librechat/data-schemas'); const db = require('~/models'); const { getAppConfig } = require('~/server/services/Config'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { getFileStrategy } = require('~/server/utils/getFileStrategy'); const SYSTEM_USER_ID = '000000000000000000000000'; +const REQUEST_SYNC_MIN_INTERVAL_MS = 5 * 60 * 1000; let appConfigRef; let runner; let scheduler; +const requestSyncsInFlight = new Set(); async function loadCurrentAppConfig() { try { @@ -29,13 +31,13 @@ async function loadCurrentAppConfig() { } } -async function getSyncConfig() { - const appConfig = await loadCurrentAppConfig(); +async function getSyncConfig(loadAppConfig = loadCurrentAppConfig) { + const appConfig = await loadAppConfig(); return appConfig?.skillSync; } -async function resolveSkillStorage({ isImage = false } = {}) { - const appConfig = await loadCurrentAppConfig(); +async function resolveSkillStorage({ isImage = false, loadAppConfig = loadCurrentAppConfig } = {}) { + const appConfig = await loadAppConfig(); const source = getFileStrategy(appConfig, { context: FileContext.skill_file, isImage }); const strategy = getStrategyFunctions(source); if (!strategy.saveBuffer) { @@ -44,8 +46,8 @@ async function resolveSkillStorage({ isImage = false } = {}) { return { source, saveBuffer: strategy.saveBuffer }; } -async function getSyntheticReq({ userId = SYSTEM_USER_ID, tenantId } = {}) { - const appConfig = await loadCurrentAppConfig(); +async function getSyntheticReq({ userId = SYSTEM_USER_ID, tenantId, loadAppConfig } = {}) { + const appConfig = await (loadAppConfig ?? loadCurrentAppConfig)(); return { config: appConfig, user: { @@ -56,9 +58,11 @@ async function getSyntheticReq({ userId = SYSTEM_USER_ID, tenantId } = {}) { }; } -function createRunner() { +function createRunner({ getConfig, loadAppConfig } = {}) { + const resolveAppConfig = loadAppConfig ?? loadCurrentAppConfig; + const resolveConfig = getConfig ?? (() => getSyncConfig(resolveAppConfig)); const createdRunner = createGitHubSkillSyncRunner({ - getConfig: getSyncConfig, + getConfig: resolveConfig, getCredentialToken: db.getSkillSyncCredentialToken, getCredentialSummary: db.getSkillSyncCredentialSummary, listCredentials: db.listSkillSyncCredentials, @@ -110,7 +114,7 @@ function createRunner() { ); }, saveBuffer: async ({ userId, buffer, fileName, basePath, isImage, tenantId }) => { - const storage = await resolveSkillStorage({ isImage }); + const storage = await resolveSkillStorage({ isImage, loadAppConfig: resolveAppConfig }); const filepath = await storage.saveBuffer({ userId: userId ?? SYSTEM_USER_ID, buffer, @@ -133,6 +137,7 @@ function createRunner() { await getSyntheticReq({ userId: file.user?.toString?.() ?? file.user ?? SYSTEM_USER_ID, tenantId: file.tenantId, + loadAppConfig: resolveAppConfig, }), file, ); @@ -144,6 +149,126 @@ function createRunner() { }; } +function parseSkillSyncConfig(raw) { + if (!raw || typeof raw !== 'object') { + return undefined; + } + const parsed = skillSyncConfigSchema.safeParse(raw); + if (!parsed.success) { + logger.warn('[GitHubSkillSync] Ignoring invalid request-scoped skill sync config', { + issues: parsed.error.flatten(), + }); + return undefined; + } + return parsed.data; +} + +function isSameSkillSyncConfig(left, right) { + return JSON.stringify(left ?? null) === JSON.stringify(right ?? null); +} + +function getRequestSkillSyncConfig(appConfig, user) { + const resolved = parseSkillSyncConfig(appConfig?.skillSync); + if (!resolved?.github?.enabled || resolved.github.sources.length === 0) { + return undefined; + } + + const base = parseSkillSyncConfig(appConfig?.config?.skillSync); + if (isSameSkillSyncConfig(resolved, base)) { + return undefined; + } + + const tenantId = typeof user?.tenantId === 'string' && user.tenantId ? user.tenantId : undefined; + return { + ...resolved, + github: { + ...resolved.github, + runOnStartup: false, + sources: resolved.github.sources.map((source) => ({ + ...source, + tenantId, + })), + }, + }; +} + +function toTimestamp(value) { + if (!value) { + return 0; + } + if (value instanceof Date) { + return value.getTime(); + } + const parsed = Date.parse(String(value)); + return Number.isNaN(parsed) ? 0 : parsed; +} + +function getLastAttemptAt(source) { + return Math.max( + toTimestamp(source.finishedAt), + toTimestamp(source.startedAt), + toTimestamp(source.updatedAt), + toTimestamp(source.lastSuccessAt), + toTimestamp(source.lastFailureAt), + ); +} + +function shouldRunRequestSync(status) { + if (!status.enabled || status.sources.length === 0) { + return false; + } + const intervalMs = Math.max( + REQUEST_SYNC_MIN_INTERVAL_MS, + (status.intervalMinutes ?? 60) * 60 * 1000, + ); + const now = Date.now(); + return status.sources.some((source) => { + if (source.status === 'running') { + return false; + } + const lastAttemptAt = getLastAttemptAt(source); + return !lastAttemptAt || now - lastAttemptAt >= intervalMs; + }); +} + +function getRequestSyncKey(config, user) { + const tenantId = user?.tenantId ?? ''; + const sources = config.github.sources + .map((source) => source.id) + .sort() + .join(','); + return `${tenantId}:${sources}`; +} + +async function maybeRunGitHubSkillSyncForRequest(req) { + const config = getRequestSkillSyncConfig(req.config, req.user); + if (!config) { + return false; + } + + const syncKey = getRequestSyncKey(config, req.user); + if (requestSyncsInFlight.has(syncKey)) { + return false; + } + + const loadAppConfig = async () => req.config; + const requestRunner = createRunner({ + getConfig: async () => config, + loadAppConfig, + }); + const status = await requestRunner.getStatus(); + if (!shouldRunRequestSync(status)) { + return false; + } + + requestSyncsInFlight.add(syncKey); + void requestRunner + .runOnce() + .catch((error) => logger.error('[GitHubSkillSync] Request-scoped sync failed:', error)) + .finally(() => requestSyncsInFlight.delete(syncKey)); + return true; +} + function initializeGitHubSkillSync(appConfig) { appConfigRef = appConfig; runner = createRunner(); @@ -171,5 +296,6 @@ function stopGitHubSkillSyncScheduler() { module.exports = { initializeGitHubSkillSync, getGitHubSkillSyncRunner, + maybeRunGitHubSkillSyncForRequest, stopGitHubSkillSyncScheduler, }; diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index 46901211d3..f77f952ee1 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -4,6 +4,8 @@ const mockGetFileStrategy = jest.fn(); const mockFindRoleByIdentifier = jest.fn(); const mockGrantPermission = jest.fn(); let mockRunnerDeps; +let mockRunnerStatus; +const mockCreatedRunners = []; jest.mock('~/server/services/Config', () => ({ getAppConfig: mockGetAppConfig, @@ -12,22 +14,71 @@ jest.mock('~/server/services/Config', () => ({ jest.mock('@librechat/api', () => ({ createGitHubSkillSyncRunner: jest.fn((deps) => { mockRunnerDeps = deps; - return { - getStatus: jest.fn(async () => deps.getConfig()), + const runner = { + getStatus: jest.fn(async () => { + if (mockRunnerStatus) { + return mockRunnerStatus; + } + const config = await deps.getConfig(); + const github = config?.github ?? {}; + return { + enabled: github.enabled ?? false, + intervalMinutes: github.intervalMinutes ?? 60, + runOnStartup: github.runOnStartup ?? false, + sources: (github.sources ?? []).map((source) => ({ + provider: 'github', + sourceId: source.id, + status: 'idle', + owner: source.owner, + repo: source.repo, + ref: source.ref, + paths: source.paths, + syncedSkillCount: 0, + syncedFileCount: 0, + deletedSkillCount: 0, + deletedFileCount: 0, + })), + credentials: [], + }; + }), runOnce: jest.fn(async () => deps.getConfig()), }; + mockCreatedRunners.push({ deps, runner }); + return runner; }), getStorageMetadata: jest.fn(() => ({})), startGitHubSkillSyncScheduler: jest.fn(() => ({ stop: jest.fn() })), })); jest.mock('@librechat/data-schemas', () => ({ + logger: { + error: jest.fn(), + warn: jest.fn(), + }, runAsSystem: jest.fn((fn) => fn()), })); jest.mock('~/models', () => ({ findRoleByIdentifier: mockFindRoleByIdentifier, grantPermission: mockGrantPermission, + getSkillSyncCredentialToken: jest.fn(), + getSkillSyncCredentialSummary: jest.fn(), + listSkillSyncCredentials: jest.fn(async () => []), + listSkillSyncStatuses: jest.fn(async () => []), + upsertSkillSyncStatus: jest.fn(), + tryAcquireSkillSyncLock: jest.fn(), + refreshSkillSyncLock: jest.fn(), + releaseSkillSyncLock: jest.fn(), + createSkill: jest.fn(), + updateSkill: jest.fn(), + getSkillById: jest.fn(), + findSkillBySourceIdentity: jest.fn(), + listSkillsBySource: jest.fn(), + listSkillFiles: jest.fn(), + getSkillFileByPath: jest.fn(), + upsertSkillFile: jest.fn(), + deleteSkillFile: jest.fn(), + deleteSkill: jest.fn(), })); jest.mock('~/server/services/Files/strategies', () => ({ getStrategyFunctions: mockGetStrategyFunctions, @@ -43,6 +94,8 @@ describe('GitHub skill sync service', () => { mockFindRoleByIdentifier.mockReset(); mockGrantPermission.mockReset(); mockRunnerDeps = undefined; + mockRunnerStatus = undefined; + mockCreatedRunners.length = 0; }); it('resolves sync config from fresh base app config for runner operations', async () => { @@ -99,6 +152,125 @@ describe('GitHub skill sync service', () => { expect(mockGetAppConfig).toHaveBeenCalledWith({ baseOnly: true }); }); + it('starts a request-scoped sync for resolved admin skillSync config', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: true, + sources: [ + { + id: 'tenant-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + tenantId: 'other-tenant', + }, + ], + }, + }; + + const service = require('./sync'); + const started = await service.maybeRunGitHubSkillSyncForRequest({ + config: { skillSync, config: {} }, + user: { id: 'user-1', tenantId: 'tenant-a' }, + }); + + const requestRunner = mockCreatedRunners[0].runner; + const requestConfig = await mockCreatedRunners[0].deps.getConfig(); + expect(started).toBe(true); + expect(requestRunner.runOnce).toHaveBeenCalledTimes(1); + expect(requestConfig.github.runOnStartup).toBe(false); + expect(requestConfig.github.sources[0]).toEqual( + expect.objectContaining({ + id: 'tenant-skills', + tenantId: 'tenant-a', + }), + ); + }); + + it('does not start a request-scoped sync for base YAML skillSync config', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: true, + sources: [ + { + id: 'base-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }, + }; + + const service = require('./sync'); + const started = await service.maybeRunGitHubSkillSyncForRequest({ + config: { skillSync, config: { skillSync } }, + user: { id: 'user-1', tenantId: 'tenant-a' }, + }); + + expect(started).toBe(false); + expect(mockCreatedRunners).toHaveLength(0); + }); + + it('does not start a request-scoped sync when the configured source is already running', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + id: 'tenant-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }, + }; + mockRunnerStatus = { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + provider: 'github', + sourceId: 'tenant-skills', + status: 'running', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + startedAt: new Date(), + syncedSkillCount: 0, + syncedFileCount: 0, + deletedSkillCount: 0, + deletedFileCount: 0, + }, + ], + credentials: [], + }; + + const service = require('./sync'); + const started = await service.maybeRunGitHubSkillSyncForRequest({ + config: { skillSync, config: {} }, + user: { id: 'user-1', tenantId: 'tenant-a' }, + }); + + expect(started).toBe(false); + expect(mockCreatedRunners[0].runner.runOnce).not.toHaveBeenCalled(); + }); + it('uses the file owner when deleting synced files from storage', async () => { const deleteFile = jest.fn(async () => undefined); const ownerId = '507f1f77bcf86cd799439011'; diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index 811c43e023..0b6719fdff 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -191,7 +191,7 @@ describe('createAdminConfigHandlers', () => { expect(savedOverrides.interface).toEqual({ modelSelect: false }); }); - it('strips base-only sections from admin overrides', async () => { + it('preserves skillSync sections in admin overrides', async () => { const { handlers, deps } = createHandlers({ upsertConfig: jest.fn().mockResolvedValue({ _id: 'c1', configVersion: 1 }), }); @@ -210,7 +210,7 @@ describe('createAdminConfigHandlers', () => { expect(res.statusCode).toBe(201); const savedOverrides = deps.upsertConfig.mock.calls[0][3]; - expect(savedOverrides.skillSync).toBeUndefined(); + expect(savedOverrides.skillSync).toEqual({ github: { enabled: true } }); expect(savedOverrides.interface).toEqual({ modelSelect: false }); }); @@ -358,7 +358,7 @@ describe('createAdminConfigHandlers', () => { expect(deps.unsetConfigField).not.toHaveBeenCalled(); }); - it('returns 200 no-op for base-only field path', async () => { + it('allows deleting skillSync field paths', async () => { const { handlers, deps } = createHandlers(); const req = mockReq({ params: { principalType: 'role', principalId: 'admin' }, @@ -369,8 +369,11 @@ describe('createAdminConfigHandlers', () => { await handlers.deleteConfigField(req, res); expect(res.statusCode).toBe(200); - expect(res.body!.message).toBeDefined(); - expect(deps.unsetConfigField).not.toHaveBeenCalled(); + expect(deps.unsetConfigField).toHaveBeenCalledWith( + 'role', + 'admin', + 'skillSync.github.enabled', + ); }); it('allows deleting interface UI field paths', async () => { @@ -452,7 +455,7 @@ describe('createAdminConfigHandlers', () => { expect(patchedFields['interface.prompts']).toBeUndefined(); }); - it('strips base-only field entries from patches', async () => { + it('preserves skillSync field entries in patches', async () => { const { handlers, deps } = createHandlers(); const req = mockReq({ params: { principalType: 'role', principalId: 'admin' }, @@ -469,7 +472,7 @@ describe('createAdminConfigHandlers', () => { expect(res.statusCode).toBe(200); const patchedFields = deps.patchConfigFields.mock.calls[0][3]; - expect(patchedFields['skillSync.github.enabled']).toBeUndefined(); + expect(patchedFields['skillSync.github.enabled']).toBe(true); expect(patchedFields['interface.modelSelect']).toBe(false); }); diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 3a0007f94d..91939c2361 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -18,7 +18,7 @@ import { REFILL_INTERVAL_UNITS } from './balance'; export const defaultSocialLogins = ['google', 'facebook', 'openid', 'github', 'discord', 'saml']; -export const BASE_ONLY_CONFIG_SECTIONS = ['skillSync'] as const; +export const BASE_ONLY_CONFIG_SECTIONS = [] as const; export const defaultRetrievalModels = [ 'gpt-4o', diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 4808304634..01b4e7e4e7 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -342,7 +342,7 @@ describe('mergeConfigOverrides', () => { expect(iface.parameters).toBe(true); }); - it('ignores base-only config sections from DB overrides', () => { + it('merges skillSync config sections from DB overrides', () => { const base = { skillSync: { github: { @@ -389,7 +389,16 @@ describe('mergeConfigOverrides', () => { const result = mergeConfigOverrides(base, configs); - expect(result.skillSync).toEqual(base.skillSync); + expect(result.skillSync?.github?.enabled).toBe(false); + expect(result.skillSync?.github?.sources).toEqual([ + { + id: 'override-source', + owner: 'other', + repo: 'skills', + paths: ['skills'], + token: '${OTHER_TOKEN}', + }, + ]); expect(result.interfaceConfig?.modelSelect).toBe(false); });