From dfef637d3d2be4668597ebcca9ac340224991e30 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 26 May 2026 18:14:13 -0700 Subject: [PATCH] fix: Address GitHub sync review cycle --- api/server/services/Skills/sync.js | 17 ++++-- api/server/services/Skills/sync.test.js | 57 ++++++++++++++++++++- packages/api/src/skills/sync/github.spec.ts | 49 +++++++++++++++++- packages/api/src/skills/sync/github.ts | 35 ++++++++++++- 4 files changed, 147 insertions(+), 11 deletions(-) diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index be47cd0b4c..fbacb64714 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -32,7 +32,7 @@ async function loadCurrentAppConfig() { async function getSyncConfig() { const appConfig = await loadCurrentAppConfig(); - return appConfig?.skillSync ?? appConfig?.config?.skillSync; + return appConfig?.skillSync; } async function resolveSkillStorage({ isImage = false } = {}) { @@ -45,13 +45,14 @@ async function resolveSkillStorage({ isImage = false } = {}) { return { source, saveBuffer: strategy.saveBuffer }; } -async function getSyntheticReq() { +async function getSyntheticReq({ userId = SYSTEM_USER_ID, tenantId } = {}) { const appConfig = await loadCurrentAppConfig(); return { config: appConfig, user: { - id: SYSTEM_USER_ID, - _id: SYSTEM_USER_ID, + id: userId, + _id: userId, + tenantId, }, }; } @@ -98,7 +99,13 @@ function createRunner() { if (!strategy.deleteFile) { return; } - await strategy.deleteFile(await getSyntheticReq(), file); + await strategy.deleteFile( + await getSyntheticReq({ + userId: file.user?.toString?.() ?? file.user ?? SYSTEM_USER_ID, + tenantId: file.tenantId, + }), + file, + ); }, }); return { diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index 48f4b4dc05..af96f3d825 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -1,4 +1,6 @@ const mockGetAppConfig = jest.fn(); +const mockGetStrategyFunctions = jest.fn(); +const mockGetFileStrategy = jest.fn(); let mockRunnerDeps; jest.mock('~/server/services/Config', () => ({ @@ -23,13 +25,17 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/models', () => ({})); jest.mock('~/server/services/PermissionService', () => ({ grantPermission: jest.fn() })); -jest.mock('~/server/services/Files/strategies', () => ({ getStrategyFunctions: jest.fn() })); -jest.mock('~/server/utils/getFileStrategy', () => ({ getFileStrategy: jest.fn() })); +jest.mock('~/server/services/Files/strategies', () => ({ + getStrategyFunctions: mockGetStrategyFunctions, +})); +jest.mock('~/server/utils/getFileStrategy', () => ({ getFileStrategy: mockGetFileStrategy })); describe('GitHub skill sync service', () => { beforeEach(() => { jest.clearAllMocks(); mockGetAppConfig.mockReset(); + mockGetStrategyFunctions.mockReset(); + mockGetFileStrategy.mockReset(); mockRunnerDeps = undefined; }); @@ -69,4 +75,51 @@ describe('GitHub skill sync service', () => { expect(mockRunnerDeps.getConfig).toBeDefined(); expect(mockGetAppConfig).toHaveBeenCalledWith({ baseOnly: true }); }); + + it('does not return raw unvalidated config.skillSync as sync config', async () => { + const rawSkillSync = { + github: { + enabled: true, + sources: 'not-an-array', + }, + }; + mockGetAppConfig.mockResolvedValue({ config: { skillSync: rawSkillSync } }); + + const service = require('./sync'); + const { runner } = service.initializeGitHubSkillSync({ config: { skillSync: rawSkillSync } }); + const result = await runner.runOnce(); + + expect(result).toBeUndefined(); + expect(mockGetAppConfig).toHaveBeenCalledWith({ baseOnly: true }); + }); + + it('uses the file owner when deleting synced files from storage', async () => { + const deleteFile = jest.fn(async () => undefined); + const ownerId = '507f1f77bcf86cd799439011'; + mockGetAppConfig.mockResolvedValue({ skillSync: undefined, paths: {} }); + mockGetStrategyFunctions.mockReturnValue({ deleteFile }); + + const service = require('./sync'); + service.initializeGitHubSkillSync({ skillSync: undefined }); + await mockRunnerDeps.deleteFile({ + filepath: `/uploads/${ownerId}/file.txt`, + source: 'local', + user: ownerId, + tenantId: 'tenant-a', + }); + + expect(deleteFile).toHaveBeenCalledWith( + expect.objectContaining({ + user: expect.objectContaining({ + id: ownerId, + _id: ownerId, + tenantId: 'tenant-a', + }), + }), + expect.objectContaining({ + user: ownerId, + tenantId: 'tenant-a', + }), + ); + }); }); diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 6879f4b973..3e72337d3e 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -10,12 +10,15 @@ import type { import { createGitHubSkillSyncRunner } from './github'; import type { GitHubSkillSyncDeps } from './github'; -function response(body: unknown, status = 200): Response { +function response(body: unknown, status = 200, headers: Record = {}): Response { + const normalizedHeaders = new Map( + Object.entries(headers).map(([key, value]) => [key.toLowerCase(), value]), + ); return { ok: status >= 200 && status < 300, status, headers: { - get: () => null, + get: (key: string) => normalizedHeaders.get(key.toLowerCase()) ?? null, }, json: async () => body, } as unknown as Response; @@ -290,6 +293,48 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('marks GitHub secondary rate limits as rate limited instead of auth failures', async () => { + const deps = createDeps({ + fetchFn: jest.fn(async () => + response( + { message: 'You have exceeded a secondary rate limit. Please wait before retrying.' }, + 403, + { 'x-ratelimit-remaining': '42' }, + ), + ) as unknown as typeof fetch, + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'GITHUB_RATE_LIMITED', + }), + ); + }); + + it('keeps non-rate-limit GitHub 403 responses classified as auth failures', async () => { + const deps = createDeps({ + fetchFn: jest.fn(async () => + response({ message: 'Resource not accessible by personal access token' }, 403, { + 'x-ratelimit-remaining': '42', + }), + ) as unknown as typeof fetch, + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'GITHUB_AUTH_FAILED', + }), + ); + }); + it('marks a source failed and skips mirror deletion when SKILL.md frontmatter is malformed', async () => { const deps = createDeps({ fetchFn: githubFetch('---\nname: [\n---\nBody'), diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 038a6167c5..2b21b7cab9 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -302,6 +302,28 @@ function buildGitHubUrl(pathname: string): string { return `${GITHUB_API_BASE}${pathname}`; } +async function readGitHubErrorMessage(response: Response): Promise { + try { + const body = (await response.json()) as { message?: unknown }; + return typeof body.message === 'string' ? body.message : undefined; + } catch { + return undefined; + } +} + +function isGitHubRateLimitResponse(params: { + status: number; + remaining: string | null; + retryAfter: string | null; + message?: string; +}): boolean { + if (params.status === 429 || params.remaining === '0' || params.retryAfter) { + return true; + } + const message = params.message?.toLowerCase() ?? ''; + return message.includes('rate limit') || message.includes('abuse detection'); +} + async function githubJson(params: { fetchFn: FetchFn; token: string; @@ -314,8 +336,17 @@ async function githubJson(params: { return (await response.json()) as T; } const remaining = response.headers.get('x-ratelimit-remaining'); - if (response.status === 401 || response.status === 403) { - const code = remaining === '0' ? 'GITHUB_RATE_LIMITED' : 'GITHUB_AUTH_FAILED'; + const retryAfter = response.headers.get('retry-after'); + const message = await readGitHubErrorMessage(response); + if (response.status === 401 || response.status === 403 || response.status === 429) { + const code = isGitHubRateLimitResponse({ + status: response.status, + remaining, + retryAfter, + message, + }) + ? 'GITHUB_RATE_LIMITED' + : 'GITHUB_AUTH_FAILED'; throw new SkillSyncError(code, `GitHub request failed with HTTP ${response.status}`); } if (response.status === 404) {