🏘️ fix: Scope Skill Sync Status (#13771)

* fix: Scope Skill Sync Status by Tenant

* fix: Preserve Unscoped Skill Sync Status

* fix: Filter Inherited Skill Sync Sources

packages/api/src/admin/skills.spec.ts

@@ -1,4 +1,5 @@
 import { SystemCapabilities } from '@librechat/data-schemas';
+import type { ISkillSyncStatus } from '@librechat/data-schemas';
 import type { NextFunction, Response } from 'express';
 import { createAdminSkillsSyncAccess, createAdminSkillsSyncHandlers } from './skills';
 
@@ -17,6 +18,36 @@ function createNext(): NextFunction & jest.Mock {
   return jest.fn() as NextFunction & jest.Mock;
 }
 
+type SourceStatus = ISkillSyncStatus & { credentialPresent: boolean };
+
+function createSourceStatus(overrides: Partial<SourceStatus> = {}): SourceStatus {
+  return {
+    provider: 'github',
+    sourceId: 'tenant-skills',
+    tenantId: 'tenant-a',
+    status: 'idle',
+    credentialKey: 'github-skills-prod',
+    credentialPresent: true,
+    owner: 'LibreChat',
+    repo: 'skills',
+    ref: 'main',
+    paths: ['skills'],
+    syncedSkillCount: 0,
+    syncedFileCount: 0,
+    deletedSkillCount: 0,
+    deletedFileCount: 0,
+    errorCode: undefined,
+    errorMessage: undefined,
+    startedAt: undefined,
+    finishedAt: undefined,
+    lastSuccessAt: undefined,
+    lastFailureAt: undefined,
+    createdAt: undefined,
+    updatedAt: undefined,
+    ...overrides,
+  };
+}
+
 function createHandlers({
   statusErrorCode,
   statusErrorMessage,
@@ -27,30 +58,10 @@ function createHandlers({
       intervalMinutes: 60,
       runOnStartup: false,
       sources: [
-        {
-          provider: 'github' as const,
-          sourceId: 'tenant-skills',
-          tenantId: 'tenant-a',
-          status: 'idle' as const,
-          credentialKey: 'github-skills-prod',
-          credentialPresent: true,
-          owner: 'LibreChat',
-          repo: 'skills',
-          ref: 'main',
-          paths: ['skills'],
-          syncedSkillCount: 0,
-          syncedFileCount: 0,
-          deletedSkillCount: 0,
-          deletedFileCount: 0,
+        createSourceStatus({
           errorCode: statusErrorCode,
           errorMessage: statusErrorMessage,
-          startedAt: undefined,
-          finishedAt: undefined,
-          lastSuccessAt: undefined,
-          lastFailureAt: undefined,
-          createdAt: undefined,
-          updatedAt: undefined,
-        },
+        }),
       ],
       credentials: [
         {
@@ -65,26 +76,13 @@ function createHandlers({
     runOnce: jest.fn(async () => ({
       status: 'completed' as const,
       sources: [
-        {
-          provider: 'github' as const,
-          sourceId: 'tenant-skills',
-          tenantId: 'tenant-a',
-          status: 'succeeded' as const,
-          credentialKey: 'github-skills-prod',
-          credentialPresent: true,
+        createSourceStatus({
+          status: 'succeeded',
           syncedSkillCount: 1,
           syncedFileCount: 2,
-          deletedSkillCount: 0,
-          deletedFileCount: 0,
           errorCode: statusErrorCode,
           errorMessage: statusErrorMessage,
-          startedAt: undefined,
-          finishedAt: undefined,
-          lastSuccessAt: undefined,
-          lastFailureAt: undefined,
-          createdAt: undefined,
-          updatedAt: undefined,
-        },
+        }),
       ],
     })),
   };
@@ -101,7 +99,13 @@ describe('createAdminSkillsSyncHandlers', () => {
     const { handlers } = createHandlers();
     const res = createResponse();
 
-    await handlers.getSyncStatus({ skillSyncCanReadCredentials: false } as never, res);
+    await handlers.getSyncStatus(
+      {
+        user: { id: 'user-1', tenantId: 'tenant-a' },
+        skillSyncCanReadCredentials: false,
+      } as never,
+      res,
+    );
 
     expect(res.status).toHaveBeenCalledWith(200);
     expect(res.json).toHaveBeenCalledWith(
@@ -121,6 +125,87 @@ describe('createAdminSkillsSyncHandlers', () => {
     );
   });
 
+  it('filters tenant-scoped status reads to the request tenant and unscoped sources', async () => {
+    const runner = {
+      getStatus: jest.fn(async () => ({
+        enabled: true,
+        intervalMinutes: 60,
+        runOnStartup: false,
+        sources: [
+          createSourceStatus({
+            sourceId: 'tenant-a-source',
+            tenantId: 'tenant-a',
+            status: 'succeeded',
+            syncedSkillCount: 4,
+            syncedFileCount: 8,
+          }),
+          createSourceStatus({
+            sourceId: 'shared-source',
+            tenantId: undefined,
+            status: 'succeeded',
+            syncedSkillCount: 2,
+            syncedFileCount: 3,
+          }),
+          createSourceStatus({
+            sourceId: 'tenant-b-source',
+            tenantId: 'tenant-b',
+            status: 'failed',
+            errorCode: 'PARSE_ERROR',
+            errorMessage: 'Failed to parse skills/private-b/SKILL.md',
+            syncedSkillCount: 7,
+            syncedFileCount: 11,
+            deletedSkillCount: 1,
+            deletedFileCount: 2,
+          }),
+        ],
+        credentials: [],
+        fineGrainedTokenRecommendation: 'Use a fine-grained token.',
+      })),
+      runOnce: jest.fn(),
+    };
+    const handlers = createAdminSkillsSyncHandlers({
+      runner,
+      upsertCredential: jest.fn(),
+      deleteCredential: jest.fn(),
+    });
+    const res = createResponse();
+
+    await handlers.getSyncStatus(
+      {
+        user: { id: 'user-1', tenantId: 'tenant-a' },
+        skillSyncCanReadCredentials: false,
+      } as never,
+      res,
+    );
+
+    expect(res.json).toHaveBeenCalledWith(
+      expect.objectContaining({
+        sources: [
+          expect.objectContaining({
+            sourceId: 'tenant-a-source',
+            tenantId: 'tenant-a',
+            status: 'succeeded',
+            syncedSkillCount: 4,
+            syncedFileCount: 8,
+          }),
+          expect.objectContaining({
+            sourceId: 'shared-source',
+            tenantId: undefined,
+            status: 'succeeded',
+            syncedSkillCount: 2,
+            syncedFileCount: 3,
+          }),
+        ],
+      }),
+    );
+    const response = res.json.mock.calls[0]?.[0] as { sources: Array<{ tenantId?: string }> };
+    expect(response.sources).toHaveLength(2);
+    expect(response.sources).toEqual([
+      expect.objectContaining({ sourceId: 'tenant-a-source' }),
+      expect.objectContaining({ sourceId: 'shared-source', tenantId: undefined }),
+    ]);
+  });
+
   it('redacts credential-related errors from tenant-scoped status reads', async () => {
     const { handlers } = createHandlers({
       statusErrorCode: 'MISSING_CREDENTIAL',
@@ -128,7 +213,13 @@ describe('createAdminSkillsSyncHandlers', () => {
     });
     const res = createResponse();
 
-    await handlers.getSyncStatus({ skillSyncCanReadCredentials: false } as never, res);
+    await handlers.getSyncStatus(
+      {
+        user: { id: 'user-1', tenantId: 'tenant-a' },
+        skillSyncCanReadCredentials: false,
+      } as never,
+      res,
+    );
 
     expect(res.json).toHaveBeenCalledWith(
       expect.objectContaining({

packages/api/src/admin/skills.ts

@@ -22,6 +22,7 @@ export type AdminSkillsRequest = Request & {
   user?: {
     _id?: Types.ObjectId;
     id?: string;
+    tenantId?: string;
   };
   skillSyncAllowServerCredentials?: boolean;
   skillSyncCanReadCredentials?: boolean;
@@ -153,6 +154,25 @@ function serializeSourceStatus(
   };
 }
 
+function getRequestTenantId(req: AdminSkillsRequest): string | undefined {
+  const tenantId = req.user?.tenantId;
+  return typeof tenantId === 'string' && tenantId ? tenantId : undefined;
+}
+
+function getVisibleSourceStatuses<T extends ISkillSyncStatus>(
+  req: AdminSkillsRequest,
+  sources: T[],
+): T[] {
+  if (req.skillSyncCanReadCredentials !== false) {
+    return sources;
+  }
+  const tenantId = getRequestTenantId(req);
+  if (!tenantId) {
+    return [];
+  }
+  return sources.filter((source) => !source.tenantId || source.tenantId === tenantId);
+}
+
 function isCredentialKey(value: unknown): value is string {
   return typeof value === 'string' && CREDENTIAL_KEY_PATTERN.test(value);
 }
@@ -332,7 +352,7 @@ export function createAdminSkillsSyncHandlers(deps: AdminSkillSyncDeps): AdminSk
       enabled: status.enabled,
       intervalMinutes: status.intervalMinutes,
       runOnStartup: status.runOnStartup,
-      sources: status.sources.map((source) =>
+      sources: getVisibleSourceStatuses(req, status.sources).map((source) =>
         serializeSourceStatus(source, { includeCredentialMetadata }),
       ),
       credentials: includeCredentialMetadata ? status.credentials.map(serializeCredential) : [],

packages/api/src/skills/sync/orchestrator.spec.ts

@@ -250,6 +250,83 @@ describe('createSkillSyncTriggerOrchestrator', () => {
     );
   });
 
+  it('filters admin base skillSync status sources without rewriting unscoped sources', async () => {
+    const config = skillSync({
+      sources: [
+        {
+          ...source,
+          id: 'tenant-a-source',
+          tenantId: 'tenant-a',
+        },
+        {
+          ...source,
+          id: 'tenant-b-source',
+          tenantId: 'tenant-b',
+        },
+        {
+          id: 'shared-source',
+          owner: 'LibreChat',
+          repo: 'skills',
+          ref: 'main',
+          paths: ['skills'],
+          token: '${GITHUB_SKILLS_TOKEN}',
+        },
+      ],
+    });
+    const { orchestrator, runners } = createHarness();
+
+    orchestrator.getRunnerForAdminRequest({
+      config: { skillSync: config, config: { skillSync: config } },
+      user: { tenantId: 'tenant-a' },
+      skillSyncAllowServerCredentials: false,
+    });
+    const runnerConfig = await runners[0].input.getConfig();
+
+    expect(runnerConfig?.github?.sources).toEqual([
+      expect.objectContaining({ id: 'tenant-a-source', tenantId: 'tenant-a' }),
+      expect.objectContaining({ id: 'shared-source', tenantId: undefined }),
+    ]);
+  });
+
+  it('filters inherited base sources when admin override changes non-source fields', async () => {
+    const baseSources = [
+      {
+        ...source,
+        id: 'tenant-a-source',
+        tenantId: 'tenant-a',
+      },
+      {
+        ...source,
+        id: 'tenant-b-source',
+        tenantId: 'tenant-b',
+      },
+      {
+        id: 'shared-source',
+        owner: 'LibreChat',
+        repo: 'skills',
+        ref: 'main',
+        paths: ['skills'],
+        token: '${GITHUB_SKILLS_TOKEN}',
+      },
+    ];
+    const base = skillSync({ intervalMinutes: 60, sources: baseSources });
+    const resolved = skillSync({ intervalMinutes: 30, sources: baseSources });
+    const { orchestrator, runners } = createHarness();
+
+    orchestrator.getRunnerForAdminRequest({
+      config: { skillSync: resolved, config: { skillSync: base } },
+      user: { tenantId: 'tenant-a' },
+      skillSyncAllowServerCredentials: false,
+    });
+    const runnerConfig = await runners[0].input.getConfig();
+
+    expect(runnerConfig?.github?.intervalMinutes).toBe(30);
+    expect(runnerConfig?.github?.sources).toEqual([
+      expect.objectContaining({ id: 'tenant-a-source', tenantId: 'tenant-a' }),
+      expect.objectContaining({ id: 'shared-source', tenantId: undefined }),
+    ]);
+  });
+
   it('does not allow admin override runners to use server credentials by default', async () => {
     const config = skillSync();
     const { orchestrator, runners } = createHarness();

packages/api/src/skills/sync/orchestrator.ts

@@ -91,7 +91,7 @@ function getRequestTenantId(user: SkillSyncRequestUser | undefined): string | un
 function withRequestTenant(
   config: SkillSyncConfigWithGitHub,
   user: SkillSyncRequestUser | undefined,
-  { disableRunOnStartup = false } = {},
+  { disableRunOnStartup = false }: { disableRunOnStartup?: boolean } = {},
 ): SkillSyncConfig {
   const tenantId = getRequestTenantId(user);
   return {
@@ -109,6 +109,44 @@ function withRequestTenant(
   };
 }
 
+function isSameGitHubSource(
+  left: ResolvedGitHubSkillSyncConfig['sources'][number] | undefined,
+  right: ResolvedGitHubSkillSyncConfig['sources'][number] | undefined,
+): boolean {
+  return JSON.stringify(left ?? null) === JSON.stringify(right ?? null);
+}
+
+function withAdminReadableTenantScope(
+  config: SkillSyncConfigWithGitHub,
+  user: SkillSyncRequestUser | undefined,
+  base: SkillSyncConfigWithGitHub | undefined,
+): SkillSyncConfig {
+  const tenantId = getRequestTenantId(user);
+  const baseSourceById = new Map(base?.github.sources.map((source) => [source.id, source]) ?? []);
+  const sources = tenantId
+    ? config.github.sources.flatMap((source) => {
+        const baseSource = baseSourceById.get(source.id);
+        if (isSameGitHubSource(source, baseSource)) {
+          if (source.tenantId && source.tenantId !== tenantId) {
+            return [];
+          }
+          return [{ ...source, tenantId: source.tenantId ? tenantId : undefined }];
+        }
+        if (source.tenantId && source.tenantId !== tenantId) {
+          return [];
+        }
+        return [{ ...source, tenantId }];
+      })
+    : [];
+  return {
+    ...config,
+    github: {
+      ...config.github,
+      sources,
+    },
+  };
+}
+
 function getRequestSkillSyncConfig(
   appConfig: SkillSyncAppConfigLike | undefined,
   user: SkillSyncRequestUser | undefined,
@@ -135,6 +173,7 @@ function getAdminRequestSkillSyncConfig(
   appConfig: SkillSyncAppConfigLike | undefined,
   user: SkillSyncRequestUser | undefined,
   logger: SkillSyncTriggerLogger,
+  { allowServerCredentials = false }: { allowServerCredentials?: boolean } = {},
 ): SkillSyncConfig | undefined {
   const resolved = parseSkillSyncConfig(appConfig?.skillSync, logger);
   if (!hasGitHubConfig(resolved)) {
@@ -142,6 +181,10 @@ function getAdminRequestSkillSyncConfig(
   }
 
   const base = parseSkillSyncConfig(appConfig?.config?.skillSync, logger);
+  const baseWithGitHub = hasGitHubConfig(base) ? base : undefined;
+  if (!allowServerCredentials) {
+    return withAdminReadableTenantScope(resolved, user, baseWithGitHub);
+  }
   if (isSameSkillSyncConfig(resolved, base)) {
     return resolved;
   }
@@ -212,11 +255,14 @@ export function createSkillSyncTriggerOrchestrator(
   const staleRunningMs = deps.staleRunningMs ?? REQUEST_SYNC_STALE_RUNNING_MS;
 
   function getRunnerForAdminRequest(request: SkillSyncRequestLike): GitHubSkillSyncRunner {
-    const config = getAdminRequestSkillSyncConfig(request.config, request.user, deps.logger);
+    const allowServerCredentials = Boolean(request.skillSyncAllowServerCredentials);
+    const config = getAdminRequestSkillSyncConfig(request.config, request.user, deps.logger, {
+      allowServerCredentials,
+    });
     return deps.createRunner({
       getConfig: async () => config,
       loadAppConfig: async () => request.config,
-      allowServerCredentials: Boolean(request.skillSyncAllowServerCredentials),
+      allowServerCredentials,
     });
   }