diff --git a/api/server/routes/admin/skills.js b/api/server/routes/admin/skills.js index 00f50fc172..0de17b9adf 100644 --- a/api/server/routes/admin/skills.js +++ b/api/server/routes/admin/skills.js @@ -42,10 +42,6 @@ function hasResolvedSkillSyncOverride(req) { return Boolean(resolved?.github && !isSameSkillSyncConfig(resolved, base)); } -function hasServerCredentialReference(config) { - return Boolean(config?.github?.sources?.some((source) => source.credentialKey || source.token)); -} - async function attachBaseSkillSyncConfig(req, res, next) { try { const baseConfig = await getAppConfig({ baseOnly: true }); @@ -107,20 +103,16 @@ async function requireSyncRunCapability(req, res, next) { }); if (canManagePlatform) { req.skillSyncAllowServerCredentials = true; + req.skillSyncCanReadCredentials = true; return next(); } - const resolved = parseSkillSyncConfig(req.config?.skillSync); if ( hasResolvedSkillSyncOverride(req) && (await hasSkillCapability(req, SystemCapabilities.MANAGE_SKILLS)) ) { - if (hasServerCredentialReference(resolved)) { - return res.status(403).json({ - message: 'Tenant-scoped skill sync runs cannot use server credentials', - }); - } - req.skillSyncAllowServerCredentials = false; - return next(); + return res.status(403).json({ + message: 'Tenant-scoped manual skill sync requires platform credential access', + }); } return res.status(403).json({ message: 'Forbidden' }); } catch { diff --git a/api/server/routes/admin/skills.test.js b/api/server/routes/admin/skills.test.js index b96649a5fc..382cac71f5 100644 --- a/api/server/routes/admin/skills.test.js +++ b/api/server/routes/admin/skills.test.js @@ -210,6 +210,7 @@ describe('admin skills sync routes', () => { const req = mockHandlers.runSync.mock.calls[0][0]; expect(req.skillSyncAllowServerCredentials).toBe(true); + expect(req.skillSyncCanReadCredentials).toBe(true); expect(req.config.config.skillSync).toEqual(skillSync); }); }); diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index be40e15ec2..601c37165c 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -239,6 +239,28 @@ describe('GitHub skill sync service', () => { ], }, }; + mockRunnerStatus = { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + provider: 'github', + sourceId: 'tenant-skills', + status: 'idle', + credentialPresent: false, + 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({ diff --git a/packages/api/src/admin/skills.spec.ts b/packages/api/src/admin/skills.spec.ts index 48330e3027..4f52d135fc 100644 --- a/packages/api/src/admin/skills.spec.ts +++ b/packages/api/src/admin/skills.spec.ts @@ -12,7 +12,10 @@ function createResponse() { }; } -function createHandlers() { +function createHandlers({ + statusErrorCode, + statusErrorMessage, +}: { statusErrorCode?: string; statusErrorMessage?: string } = {}) { const runner = { getStatus: jest.fn(async () => ({ enabled: true, @@ -34,8 +37,8 @@ function createHandlers() { syncedFileCount: 0, deletedSkillCount: 0, deletedFileCount: 0, - errorCode: undefined, - errorMessage: undefined, + errorCode: statusErrorCode, + errorMessage: statusErrorMessage, startedAt: undefined, finishedAt: undefined, lastSuccessAt: undefined, @@ -68,8 +71,8 @@ function createHandlers() { syncedFileCount: 2, deletedSkillCount: 0, deletedFileCount: 0, - errorCode: undefined, - errorMessage: undefined, + errorCode: statusErrorCode, + errorMessage: statusErrorMessage, startedAt: undefined, finishedAt: undefined, lastSuccessAt: undefined, @@ -109,8 +112,32 @@ describe('createAdminSkillsSyncHandlers', () => { ); }); + it('redacts credential-related errors from tenant-scoped status reads', async () => { + const { handlers } = createHandlers({ + statusErrorCode: 'MISSING_CREDENTIAL', + statusErrorMessage: 'Missing GitHub token environment variable "GITHUB_SKILLS_TOKEN"', + }); + const res = createResponse(); + + await handlers.getSyncStatus({ skillSyncCanReadCredentials: false } as never, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + sources: [ + expect.objectContaining({ + errorCode: 'MISSING_CREDENTIAL', + errorMessage: 'GitHub skill sync credentials are not available', + }), + ], + }), + ); + }); + it('includes credential summaries and source credential metadata for platform status reads', async () => { - const { handlers } = createHandlers(); + const { handlers } = createHandlers({ + statusErrorCode: 'MISSING_CREDENTIAL', + statusErrorMessage: 'Missing GitHub credential "github-skills-prod"', + }); const res = createResponse(); await handlers.getSyncStatus({ skillSyncCanReadCredentials: true } as never, res); @@ -122,6 +149,7 @@ describe('createAdminSkillsSyncHandlers', () => { expect.objectContaining({ credentialKey: 'github-skills-prod', credentialPresent: true, + errorMessage: 'Missing GitHub credential "github-skills-prod"', }), ], }), @@ -129,10 +157,19 @@ describe('createAdminSkillsSyncHandlers', () => { }); it('omits source credential metadata from tenant-scoped manual run responses', async () => { - const { handlers } = createHandlers(); + const { handlers } = createHandlers({ + statusErrorCode: 'MISSING_CREDENTIAL', + statusErrorMessage: 'Missing GitHub credential "github-skills-prod"', + }); const res = createResponse(); - await handlers.runSync({ skillSyncAllowServerCredentials: false } as never, res); + await handlers.runSync( + { + skillSyncAllowServerCredentials: true, + skillSyncCanReadCredentials: false, + } as never, + res, + ); expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ @@ -140,6 +177,7 @@ describe('createAdminSkillsSyncHandlers', () => { expect.objectContaining({ credentialKey: undefined, credentialPresent: false, + errorMessage: 'GitHub skill sync credentials are not available', }), ], }), diff --git a/packages/api/src/admin/skills.ts b/packages/api/src/admin/skills.ts index a64c98190e..d236b31be9 100644 --- a/packages/api/src/admin/skills.ts +++ b/packages/api/src/admin/skills.ts @@ -52,6 +52,25 @@ function serializeCredential( }; } +function isCredentialError(status: ISkillSyncStatus): boolean { + if (status.errorCode === 'MISSING_CREDENTIAL') { + return true; + } + return /credential|token environment variable|server github credentials/i.test( + status.errorMessage ?? '', + ); +} + +function serializeErrorMessage( + status: ISkillSyncStatus, + { includeCredentialMetadata }: { includeCredentialMetadata: boolean }, +): string | undefined { + if (includeCredentialMetadata || !isCredentialError(status)) { + return status.errorMessage; + } + return 'GitHub skill sync credentials are not available'; +} + function serializeSourceStatus( status: ISkillSyncStatus & { credentialPresent?: boolean }, { includeCredentialMetadata = true }: { includeCredentialMetadata?: boolean } = {}, @@ -72,7 +91,7 @@ function serializeSourceStatus( lastSuccessAt: toIso(status.lastSuccessAt), lastFailureAt: toIso(status.lastFailureAt), errorCode: status.errorCode, - errorMessage: status.errorMessage, + errorMessage: serializeErrorMessage(status, { includeCredentialMetadata }), syncedSkillCount: status.syncedSkillCount, syncedFileCount: status.syncedFileCount, deletedSkillCount: status.deletedSkillCount, @@ -116,7 +135,7 @@ export function createAdminSkillsSyncHandlers(deps: AdminSkillSyncDeps) { } async function runSync(req: AdminSkillsRequest, res: Response) { - const includeCredentialMetadata = req.skillSyncAllowServerCredentials === true; + const includeCredentialMetadata = req.skillSyncCanReadCredentials === true; const result = await getRunner(req).runOnce(); const response: TGitHubSkillSyncManualRunResponse = { status: result.status, diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index fc7c89a086..97c83a4265 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -8,6 +8,8 @@ import type { CreateSkillResult, ISkillSyncStatus, SkillSyncStatusInput, + UpdateSkillInput, + UpdateSkillResult, } from '@librechat/data-schemas'; import type { GitHubSkillSyncDeps } from './github'; import { DEFAULT_SKILL_IMPORT_LIMITS } from '../limits'; @@ -306,6 +308,75 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('fails duplicate discovered skill names before publishing partial mirrors', async () => { + const duplicateFetch = jest.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes('/commits/')) { + return response({ sha: 'commit-sha', commit: { tree: { sha: 'tree-sha' } } }); + } + if (url.includes('/git/trees/tree-sha')) { + return response({ + sha: 'tree-sha', + truncated: false, + tree: [ + { + path: 'skills', + mode: '040000', + type: 'tree', + sha: 'skills-tree-sha', + url: 'https://api.github.test/tree/skills', + }, + ], + }); + } + if (url.includes('/git/trees/skills-tree-sha')) { + return response({ + sha: 'skills-tree-sha', + truncated: false, + tree: [ + { + path: 'research/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'skill-a-sha', + size: 50, + url: 'https://api.github.test/blob/skill-a', + }, + { + path: 'analysis/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'skill-b-sha', + size: 50, + url: 'https://api.github.test/blob/skill-b', + }, + ], + }); + } + if (url.includes('/git/blobs/skill-a-sha')) { + return response(blob('---\nname: duplicate\ndescription: First\n---\nBody')); + } + if (url.includes('/git/blobs/skill-b-sha')) { + return response(blob('---\nname: duplicate\ndescription: Second\n---\nBody')); + } + return response({ message: 'not found' }, 404); + }) as unknown as typeof fetch; + const deps = createDeps({ fetchFn: duplicateFetch }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.createSkill).not.toHaveBeenCalled(); + expect(deps.updateSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'DUPLICATE_SKILL_NAME', + errorMessage: 'GitHub source "librechat-skills" contains multiple skills named "duplicate"', + }), + ); + }); + it('discovers nested skill roots within the configured discovery depth', async () => { const skillMarkdown = '---\nname: tdd\ndescription: Test-driven development\n---\nBody'; const fetchFn = jest.fn(async (input: RequestInfo | URL) => { @@ -721,6 +792,85 @@ describe('createGitHubSkillSyncRunner', () => { expect(deps.deleteSkill).toHaveBeenCalledWith(staleId.toString()); }); + it('deletes stale name-conflicting mirrors before applying same-commit renames', async () => { + const staleId = new Types.ObjectId(); + const existingId = new Types.ObjectId(); + const author = makeSourceAuthorId(); + const existingSkill = ( + upstreamId: string, + _id: Types.ObjectId, + name: string, + ): ISkill & { _id: Types.ObjectId } => { + const skill = makeSkill({ + name, + description: `${name} skill`, + body: 'Old body', + author, + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { provider: 'github', sourceId: 'librechat-skills', upstreamId }, + }); + skill._id = _id; + return skill; + }; + const staleSkill = existingSkill('librechat-skills:skills/removed', staleId, 'renamed'); + const syncedSkill = existingSkill('librechat-skills:skills/research', existingId, 'research'); + const existingById = new Map([[existingId.toString(), syncedSkill]]); + const deletedIds = new Set(); + const listSkillsBySource = jest.fn(async () => + [staleSkill, syncedSkill].filter((skill) => !deletedIds.has(skill._id.toString())), + ); + const deleteSkill = jest.fn(async (id: string) => { + deletedIds.add(id); + return { deleted: true }; + }); + const updateSkill = jest.fn( + async ({ + id, + update, + }: { + id: string; + expectedVersion: number; + update: UpdateSkillInput; + }): Promise => { + if (!deletedIds.has(staleId.toString()) && update.name === 'renamed') { + throw new Error('duplicate key'); + } + const skill = existingById.get(id); + if (!skill) { + return { status: 'not_found' as const }; + } + const updated = { ...skill, ...update, version: skill.version + 1 }; + existingById.set(id, updated); + return { status: 'updated' as const, skill: updated, warnings: [] }; + }, + ); + const deps = createDeps({ + fetchFn: githubFetch('---\nname: renamed\ndescription: Renamed skill\n---\nBody'), + findSkillBySourceIdentity: jest.fn(async ({ upstreamId }) => + upstreamId === 'librechat-skills:skills/research' ? syncedSkill : null, + ), + getSkillById: jest.fn(async (id) => existingById.get(id.toString()) ?? null), + listSkillsBySource, + deleteSkill, + updateSkill, + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deleteSkill).toHaveBeenCalledWith(staleId.toString()); + expect(updateSkill).toHaveBeenCalledWith( + expect.objectContaining({ + id: existingId.toString(), + update: expect.objectContaining({ name: 'renamed' }), + }), + ); + expect(deleteSkill.mock.invocationCallOrder[0]).toBeLessThan( + updateSkill.mock.invocationCallOrder[0], + ); + }); + it("does not mirror-delete another tenant's skills from an ambient source run", async () => { const ambientStaleId = new Types.ObjectId(); const otherTenantId = new Types.ObjectId(); diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 6a56d3f7ae..5bb6b687d1 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -97,6 +97,11 @@ type PreparedExistingRemoteSkill = PreparedRemoteSkill & { existing: ISkill & { _id: Types.ObjectId }; }; +type PreparedDiscoveredSkill = { + discovered: DiscoveredSkill; + prepared: PreparedRemoteSkill; +}; + type SaveBufferResult = { filepath: string; source: string; @@ -1057,6 +1062,96 @@ function findMovedSourceSkill(params: { ); } +function getMirrorNameKey(params: { + tenantId?: string; + author: string; + name: string | undefined; +}): string { + return `${params.tenantId ?? ''}:${params.author}:${params.name ?? ''}`; +} + +function assertNoDuplicatePreparedSkillNames( + source: SkillSyncGitHubSourceConfig, + preparedSkills: PreparedDiscoveredSkill[], +): void { + const sourceTenantId = source.tenantId ?? undefined; + const seen = new Map(); + for (const { discovered, prepared } of preparedSkills) { + const key = getMirrorNameKey({ + tenantId: sourceTenantId, + author: prepared.createInput.author.toString(), + name: prepared.createInput.name, + }); + const previousRoot = seen.get(key); + if (previousRoot) { + throw new SkillSyncError( + 'DUPLICATE_SKILL_NAME', + `GitHub source "${source.id}" contains multiple skills named "${prepared.createInput.name}"`, + ); + } + seen.set(key, discovered.rootPath); + } +} + +async function deleteNameConflictingStaleSkills(params: { + deps: GitHubSkillSyncDeps; + source: SkillSyncGitHubSourceConfig; + preparedSkills: PreparedDiscoveredSkill[]; + existingSyncedSkills: Array; + discoveredUpstreamIds: Set; + assertNotCancelled: AssertNotCancelled; +}): Promise<{ + remainingSkills: Array; + deletedSkillCount: number; + deletedFileCount: number; +}> { + const sourceTenantId = params.source.tenantId ?? undefined; + const conflictingUpdateKeys = new Set( + params.preparedSkills + .filter(({ prepared }) => prepared.existing) + .map(({ prepared }) => + getMirrorNameKey({ + tenantId: sourceTenantId, + author: prepared.createInput.author.toString(), + name: prepared.update.name, + }), + ), + ); + if (conflictingUpdateKeys.size === 0) { + return { + remainingSkills: params.existingSyncedSkills, + deletedSkillCount: 0, + deletedFileCount: 0, + }; + } + + const remainingSkills: Array = []; + let deletedSkillCount = 0; + let deletedFileCount = 0; + for (const skill of params.existingSyncedSkills) { + params.assertNotCancelled(); + const upstreamId = getSourceMetadataString(skill, 'upstreamId'); + const shouldDelete = + (skill.tenantId ?? undefined) === sourceTenantId && + (!upstreamId || !params.discoveredUpstreamIds.has(upstreamId)) && + conflictingUpdateKeys.has( + getMirrorNameKey({ + tenantId: sourceTenantId, + author: skill.author.toString(), + name: skill.name, + }), + ); + if (!shouldDelete) { + remainingSkills.push(skill); + continue; + } + deletedFileCount += await deleteSyncedSkill(params.deps, skill); + deletedSkillCount++; + } + + return { remainingSkills, deletedSkillCount, deletedFileCount }; +} + async function syncSkillFiles(params: { deps: GitHubSkillSyncDeps; token: string; @@ -1256,6 +1351,7 @@ async function syncSource(params: { deletedFileCount: 0, }; const syncedAt = new Date(); + const preparedSkills: PreparedDiscoveredSkill[] = []; for (const discovered of discoveredSkills) { assertNotCancelled(); @@ -1277,6 +1373,27 @@ async function syncSource(params: { commitSha: commit.sha, syncedAt, }); + preparedSkills.push({ discovered, prepared }); + } + + const discoveredUpstreamIds = new Set( + preparedSkills.map(({ discovered }) => makeUpstreamId(source, discovered.rootPath)), + ); + assertNoDuplicatePreparedSkillNames(source, preparedSkills); + const staleConflictCleanup = await deleteNameConflictingStaleSkills({ + deps, + source, + preparedSkills, + existingSyncedSkills: await getExistingSyncedSkills(), + discoveredUpstreamIds, + assertNotCancelled, + }); + existingSyncedSkills = staleConflictCleanup.remainingSkills; + counts.deletedSkillCount += staleConflictCleanup.deletedSkillCount; + counts.deletedFileCount += staleConflictCleanup.deletedFileCount; + + for (const { discovered, prepared } of preparedSkills) { + assertNotCancelled(); const movedExisting = prepared.existing ? null : findMovedSourceSkill({