fix: tighten skill sync trigger safeguards

This commit is contained in:
Danny Avila 2026-06-04 22:03:20 -04:00
parent 390a8deb6f
commit a4e51ecc7d
7 changed files with 145 additions and 25 deletions

View file

@ -32,6 +32,9 @@ jest.mock('@librechat/api', () => {
provider: 'github',
sourceId: source.id,
status: 'idle',
credentialPresent:
deps.allowServerCredentials !== false &&
Boolean(source.credentialKey || source.token),
owner: source.owner,
repo: source.repo,
ref: source.ref,
@ -175,6 +178,28 @@ 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({
@ -196,6 +221,36 @@ describe('GitHub skill sync service', () => {
);
});
it('does not auto-start request-scoped sync when server credentials are unavailable', 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}',
},
],
},
};
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].deps.allowServerCredentials).toBe(false);
expect(mockCreatedRunners[0].runner.runOnce).not.toHaveBeenCalled();
});
it('does not start a request-scoped sync for base YAML skillSync config', async () => {
const skillSync = {
github: {
@ -317,6 +372,7 @@ describe('GitHub skill sync service', () => {
provider: 'github',
sourceId: 'tenant-skills',
status: 'running',
credentialPresent: true,
owner: 'LibreChat',
repo: 'skills',
ref: 'main',
@ -368,6 +424,7 @@ describe('GitHub skill sync service', () => {
provider: 'github',
sourceId: 'tenant-skills',
status: 'running',
credentialPresent: true,
owner: 'LibreChat',
repo: 'skills',
ref: 'main',

View file

@ -433,7 +433,7 @@ describe('createGitHubSkillSyncRunner', () => {
}
});
it('does not list or resolve server credentials when server credentials are disabled', async () => {
it('does not list or resolve server credentials and skips runs when they are disabled', async () => {
const previousToken = process.env.GITHUB_SKILLS_TOKEN;
process.env.GITHUB_SKILLS_TOKEN = 'github_pat_from_env';
const getCredentialToken = jest.fn(async () => 'github_pat_from_db');
@ -489,22 +489,22 @@ describe('createGitHubSkillSyncRunner', () => {
credentialPresent: false,
}),
]);
expect(result.status).toBe('failed');
expect(result.status).toBe('skipped');
expect(result.message).toBe(
'GitHub skill sync credentials are not available for this runner',
);
expect(result.sources).toEqual([
expect.objectContaining({
sourceId: 'librechat-skills',
status: 'failed',
errorCode: 'MISSING_CREDENTIAL',
}),
expect.objectContaining({ sourceId: 'librechat-skills', credentialPresent: false }),
expect.objectContaining({
sourceId: 'stored-credential-skills',
status: 'failed',
errorCode: 'MISSING_CREDENTIAL',
credentialPresent: false,
}),
]);
expect(listCredentials).not.toHaveBeenCalled();
expect(getCredentialToken).not.toHaveBeenCalled();
expect(deps.fetchFn).not.toHaveBeenCalled();
expect(deps.tryAcquireLock).not.toHaveBeenCalled();
expect(deps.upsertStatus).not.toHaveBeenCalled();
} finally {
if (previousToken == null) {
delete process.env.GITHUB_SKILLS_TOKEN;
@ -592,9 +592,8 @@ describe('createGitHubSkillSyncRunner', () => {
expect(deps.upsertStatus).toHaveBeenCalledWith(
expect.objectContaining({ sourceId: 'librechat-skills', tenantId: 'tenant-a' }),
);
expect(deps.tryAcquireLock).toHaveBeenCalledWith(
expect.objectContaining({ tenantId: 'tenant-a' }),
);
const [lockParams] = (deps.tryAcquireLock as jest.Mock).mock.calls[0];
expect(lockParams).not.toHaveProperty('tenantId');
});
it('matches stored source status by tenant and source id', async () => {

View file

@ -760,11 +760,6 @@ function makeStatusKey(sourceId: string, tenantId?: string): string {
return `${tenantId ?? ''}:${sourceId}`;
}
function getLockTenantId(sources: SkillSyncGitHubSourceConfig[]): string | undefined {
const tenantIds = new Set(sources.map((source) => source.tenantId).filter(Boolean));
return tenantIds.size === 1 ? [...tenantIds][0] : undefined;
}
async function ensurePublicViewer(
deps: GitHubSkillSyncDeps,
skillId: Types.ObjectId,
@ -1540,13 +1535,22 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
if (!github.enabled || github.sources.length === 0) {
return { status: 'skipped', message: 'GitHub skill sync is disabled', sources: [] };
}
const allowServerCredentials = deps.allowServerCredentials !== false;
if (!allowServerCredentials) {
const status = await getStatus();
if (!status.sources.some((source) => source.credentialPresent)) {
return {
status: 'skipped',
message: 'GitHub skill sync credentials are not available for this runner',
sources: status.sources,
};
}
}
const lockOwner = `${lockOwnerPrefix}:${crypto.randomUUID().replace(/-/g, '').slice(0, 12)}`;
const lockTenantId = getLockTenantId(github.sources);
const acquired = await deps.tryAcquireLock({
provider: PROVIDER,
lockOwner,
leaseMs: LOCK_LEASE_MS,
tenantId: lockTenantId,
});
if (!acquired) {
const status = await getStatus();
@ -1569,7 +1573,6 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
provider: PROVIDER,
lockOwner,
leaseMs: LOCK_LEASE_MS,
tenantId: lockTenantId,
})
.then((refreshed) => {
if (!refreshed) {
@ -1603,7 +1606,7 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
};
} finally {
clearInterval(refreshTimer);
await deps.releaseLock({ provider: PROVIDER, lockOwner, tenantId: lockTenantId });
await deps.releaseLock({ provider: PROVIDER, lockOwner });
}
}

View file

@ -30,7 +30,10 @@ function skillSync(
};
}
function statusFromConfig(config: SkillSyncConfig | undefined): RunnerStatus {
function statusFromConfig(
config: SkillSyncConfig | undefined,
{ allowServerCredentials = true }: { allowServerCredentials?: boolean } = {},
): RunnerStatus {
const github = config?.github;
return {
enabled: github?.enabled ?? false,
@ -43,7 +46,9 @@ function statusFromConfig(config: SkillSyncConfig | undefined): RunnerStatus {
tenantId: configuredSource.tenantId,
status: 'idle',
credentialKey: configuredSource.credentialKey,
credentialPresent: Boolean(configuredSource.credentialKey || configuredSource.token),
credentialPresent:
allowServerCredentials &&
Boolean(configuredSource.credentialKey || configuredSource.token),
owner: configuredSource.owner,
repo: configuredSource.repo,
ref: configuredSource.ref,
@ -66,6 +71,16 @@ function statusFromConfig(config: SkillSyncConfig | undefined): RunnerStatus {
};
}
function withRunnableCredentials(status: RunnerStatus): RunnerStatus {
return {
...status,
sources: status.sources.map((configuredSource) => ({
...configuredSource,
credentialPresent: true,
})),
};
}
function completedRun(): RunnerRunResult {
return { status: 'completed', sources: [] };
}
@ -99,7 +114,13 @@ function createHarness(
};
const createRunner = jest.fn((input: SkillSyncTriggerRunnerFactoryInput) => {
const runner: GitHubSkillSyncRunner = {
getStatus: jest.fn(async () => options.status ?? statusFromConfig(await input.getConfig())),
getStatus: jest.fn(
async () =>
options.status ??
statusFromConfig(await input.getConfig(), {
allowServerCredentials: input.allowServerCredentials !== false,
}),
),
runOnce: jest.fn(options.runOnce ?? (async () => completedRun())),
};
runners.push({ input, runner });
@ -115,7 +136,9 @@ function createHarness(
describe('createSkillSyncTriggerOrchestrator', () => {
it('starts request sync from resolved admin skillSync config and derives tenant from the request', async () => {
const config = skillSync();
const { orchestrator, runners } = createHarness();
const { orchestrator, runners } = createHarness({
status: withRunnableCredentials(statusFromConfig(config)),
});
const started = await orchestrator.maybeRunForRequest({
config: { skillSync: config, config: {} },
@ -132,6 +155,20 @@ describe('createSkillSyncTriggerOrchestrator', () => {
);
});
it('does not auto-start request sync when only server credentials are configured', async () => {
const config = skillSync();
const { orchestrator, runners } = createHarness();
const started = await orchestrator.maybeRunForRequest({
config: { skillSync: config, config: {} },
user: { tenantId: 'tenant-a' },
});
expect(started).toBe(false);
expect(runners[0].input.allowServerCredentials).toBe(false);
expect(runners[0].runner.runOnce).not.toHaveBeenCalled();
});
it('does not start request sync for base YAML skillSync config', async () => {
const config = skillSync();
const { createRunner, orchestrator } = createHarness();
@ -209,6 +246,7 @@ describe('createSkillSyncTriggerOrchestrator', () => {
{
...statusFromConfig(config).sources[0],
status: 'running',
credentialPresent: true,
startedAt: new Date(Date.now() - 40 * 60 * 1000),
},
],
@ -228,6 +266,7 @@ describe('createSkillSyncTriggerOrchestrator', () => {
const pendingRun = deferred<RunnerRunResult>();
const config = skillSync({ runOnStartup: false });
const { orchestrator, runners } = createHarness({
status: withRunnableCredentials(statusFromConfig(config)),
runOnce: () => pendingRun.promise,
});

View file

@ -173,6 +173,9 @@ function shouldRunRequestSync(
const intervalMs = Math.max(minIntervalMs, (status.intervalMinutes ?? 60) * 60 * 1000);
const now = Date.now();
return status.sources.some((source) => {
if (!source.credentialPresent) {
return false;
}
if (source.status === 'running') {
const startedAt = toTimestamp(source.startedAt);
return Boolean(startedAt && now - startedAt >= staleRunningMs);

View file

@ -139,6 +139,23 @@ function makeSkillInput(overrides: Record<string, unknown> = {}) {
};
}
describe('Skill schema indexes', () => {
it('supports GitHub sync source metadata lookups', () => {
const indexSpecs = Skill.schema.indexes().map(([fields]) => fields);
expect(indexSpecs).toContainEqual({
source: 1,
'sourceMetadata.upstreamId': 1,
tenantId: 1,
});
expect(indexSpecs).toContainEqual({
source: 1,
'sourceMetadata.sourceId': 1,
tenantId: 1,
});
});
});
describe('skill validation helpers', () => {
it('rejects names starting with reserved brand prefixes', () => {
expect(validateSkillName('anthropic-helper').some((i) => i.code === 'RESERVED_PREFIX')).toBe(

View file

@ -229,5 +229,7 @@ skillSchema.index({ author: 1, tenantId: 1 });
skillSchema.index({ category: 1, updatedAt: -1 });
skillSchema.index({ updatedAt: -1, _id: 1 });
skillSchema.index({ name: 1, author: 1, tenantId: 1 }, { unique: true });
skillSchema.index({ source: 1, 'sourceMetadata.upstreamId': 1, tenantId: 1 });
skillSchema.index({ source: 1, 'sourceMetadata.sourceId': 1, tenantId: 1 });
export default skillSchema;