From 03ae9003a7f4764fcd69d47d78fd5c3bfa11da71 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 7 Jun 2026 22:17:32 -0400 Subject: [PATCH] fix: resolve codex skill sync review findings --- api/server/services/Skills/sync.js | 5 +- api/server/services/Skills/sync.test.js | 4 +- packages/api/src/skills/sync/github.spec.ts | 73 +++++++++++++++++++ packages/api/src/skills/sync/github.ts | 3 +- .../api/src/skills/sync/orchestrator.spec.ts | 15 ++++ packages/api/src/skills/sync/orchestrator.ts | 2 +- 6 files changed, 96 insertions(+), 6 deletions(-) diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index 1e2f940206..407d10d5c4 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -176,7 +176,10 @@ function getGitHubSkillSyncRunnerForRequest(req) { async function maybeRunGitHubSkillSyncForRequest(req) { const baseConfig = await loadCurrentAppConfig(); - return triggerOrchestrator.maybeRunForRequest(withBaseSkillSyncConfig(req, baseConfig)); + return triggerOrchestrator.maybeRunForRequest({ + ...withBaseSkillSyncConfig(req, baseConfig), + skillSyncAllowServerCredentials: true, + }); } function initializeGitHubSkillSync(appConfig) { diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index 601c37165c..7722372808 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -210,7 +210,7 @@ describe('GitHub skill sync service', () => { const requestRunner = mockCreatedRunners[0].runner; const requestConfig = await mockCreatedRunners[0].deps.getConfig(); expect(started).toBe(true); - expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false); + expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(true); expect(requestRunner.runOnce).toHaveBeenCalledTimes(1); expect(requestConfig.github.runOnStartup).toBe(false); expect(requestConfig.github.sources[0]).toEqual( @@ -269,7 +269,7 @@ describe('GitHub skill sync service', () => { }); expect(started).toBe(false); - expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false); + expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(true); expect(mockCreatedRunners[0].runner.runOnce).not.toHaveBeenCalled(); }); diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 9cf7ae4dde..a5a04583f0 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -377,6 +377,79 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('fails duplicate root and nested 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: 'SKILL.md', + mode: '100644', + type: 'blob', + sha: 'root-skill-sha', + size: 50, + url: 'https://api.github.test/blob/root-skill', + }, + { + path: 'child/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'child-skill-sha', + size: 50, + url: 'https://api.github.test/blob/child-skill', + }, + ], + }); + } + if (url.includes('/git/blobs/root-skill-sha')) { + return response(blob('---\nname: duplicate\ndescription: Root\n---\nBody')); + } + if (url.includes('/git/blobs/child-skill-sha')) { + return response(blob('---\nname: duplicate\ndescription: Child\n---\nBody')); + } + return response({ message: 'not found' }, 404); + }) as unknown as typeof fetch; + const deps = createDeps({ + fetchFn: duplicateFetch, + getConfig: () => ({ + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + id: 'librechat-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: [''], + credentialKey: 'github-skills-prod', + }, + ], + }, + }), + }); + 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) => { diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 5bb6b687d1..72ebbf6c52 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -1082,8 +1082,7 @@ function assertNoDuplicatePreparedSkillNames( author: prepared.createInput.author.toString(), name: prepared.createInput.name, }); - const previousRoot = seen.get(key); - if (previousRoot) { + if (seen.has(key)) { throw new SkillSyncError( 'DUPLICATE_SKILL_NAME', `GitHub source "${source.id}" contains multiple skills named "${prepared.createInput.name}"`, diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index 08400339f7..6638330bd9 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -169,6 +169,21 @@ describe('createSkillSyncTriggerOrchestrator', () => { expect(runners[0].runner.runOnce).not.toHaveBeenCalled(); }); + it('starts request sync with server credentials when the caller opts in', async () => { + const config = skillSync(); + const { orchestrator, runners } = createHarness(); + + const started = await orchestrator.maybeRunForRequest({ + config: { skillSync: config, config: {} }, + user: { tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: true, + }); + + expect(started).toBe(true); + expect(runners[0].input.allowServerCredentials).toBe(true); + expect(runners[0].runner.runOnce).toHaveBeenCalledTimes(1); + }); + it('does not start request sync for base YAML skillSync config', async () => { const config = skillSync(); const { createRunner, orchestrator } = createHarness(); diff --git a/packages/api/src/skills/sync/orchestrator.ts b/packages/api/src/skills/sync/orchestrator.ts index 59fc10e9e6..a4b14d261a 100644 --- a/packages/api/src/skills/sync/orchestrator.ts +++ b/packages/api/src/skills/sync/orchestrator.ts @@ -225,7 +225,7 @@ export function createSkillSyncTriggerOrchestrator(deps: SkillSyncTriggerOrchest const requestRunner = deps.createRunner({ getConfig: async () => config, loadAppConfig: async () => request.config, - allowServerCredentials: false, + allowServerCredentials: Boolean(request.skillSyncAllowServerCredentials), }); const status = await requestRunner.getStatus(); if (!shouldRunRequestSync(status, { minIntervalMs, staleRunningMs })) {