fix: Address GitHub sync review cycle

This commit is contained in:
Danny Avila 2026-05-26 18:14:13 -07:00
parent 432227055c
commit dfef637d3d
No known key found for this signature in database
GPG key ID: BF31EEB2C5CA0956
4 changed files with 147 additions and 11 deletions

View file

@ -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 {

View file

@ -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',
}),
);
});
});

View file

@ -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<string, string> = {}): 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'),

View file

@ -302,6 +302,28 @@ function buildGitHubUrl(pathname: string): string {
return `${GITHUB_API_BASE}${pathname}`;
}
async function readGitHubErrorMessage(response: Response): Promise<string | undefined> {
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<T>(params: {
fetchFn: FetchFn;
token: string;
@ -314,8 +336,17 @@ async function githubJson<T>(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) {