From d6d402dce18436a605888e7d0b4c2039bb84fb89 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 8 Jun 2026 00:01:36 -0400 Subject: [PATCH] fix: resolve codex skill sync edge findings --- packages/api/src/skills/sync/github.spec.ts | 112 ++++++++++++++++++ packages/api/src/skills/sync/github.ts | 10 +- .../api/src/skills/sync/orchestrator.spec.ts | 17 +++ packages/api/src/skills/sync/orchestrator.ts | 4 +- 4 files changed, 137 insertions(+), 6 deletions(-) diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 406ddb5c9e..d0c8e946a6 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -1461,6 +1461,118 @@ describe('createGitHubSkillSyncRunner', () => { expect(deps.deleteSkill).not.toHaveBeenCalledWith(otherTenantSkill._id.toString()); }); + it('does not match still-discovered mirrors as moved skills when new skills sync first', async () => { + const newSkillMarkdown = '---\nname: research\ndescription: New research skill\n---\nNew'; + const renamedSkillMarkdown = '---\nname: renamed\ndescription: Renamed skill\n---\nRenamed'; + const fetchFn = 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: 'new/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'new-skill-sha', + size: Buffer.byteLength(newSkillMarkdown), + url: 'https://api.github.test/blob/new-skill', + }, + { + path: 'research/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'renamed-skill-sha', + size: Buffer.byteLength(renamedSkillMarkdown), + url: 'https://api.github.test/blob/renamed-skill', + }, + ], + }); + } + if (url.includes('/git/blobs/new-skill-sha')) { + return response(blob(newSkillMarkdown)); + } + if (url.includes('/git/blobs/renamed-skill-sha')) { + return response(blob(renamedSkillMarkdown)); + } + return response({ message: 'not found' }, 404); + }) as unknown as typeof fetch; + const existing = makeSkill({ + name: 'research', + description: 'Old research skill', + body: 'Old body', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/research', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/research', + }, + }) as ISkill & { _id: Types.ObjectId }; + const deps = createDeps({ + fetchFn, + findSkillBySourceIdentity: jest.fn(async ({ upstreamId }) => + upstreamId === 'librechat-skills:skills/research' ? existing : null, + ), + listSkillsBySource: jest.fn(async () => [existing]), + getSkillById: jest.fn(async (id) => + id.toString() === existing._id.toString() ? existing : null, + ), + updateSkill: jest.fn(async ({ update }) => ({ + status: 'updated' as const, + skill: { ...existing, ...update, version: existing.version + 1 }, + warnings: [], + })), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'research', + sourceMetadata: expect.objectContaining({ + upstreamId: 'librechat-skills:skills/new', + }), + }), + ); + expect(deps.updateSkill).toHaveBeenCalledWith( + expect.objectContaining({ + id: existing._id.toString(), + update: expect.objectContaining({ + name: 'renamed', + sourceMetadata: expect.objectContaining({ + upstreamId: 'librechat-skills:skills/research', + }), + }), + }), + ); + }); + it('reuses a same-named source mirror when a skill moves configured paths', async () => { const existing = makeSkill({ name: 'research', diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 505b6bb88e..75289563ed 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -1111,7 +1111,7 @@ function findMovedSourceSkill(params: { source: SkillSyncGitHubSourceConfig; prepared: PreparedRemoteSkill; existingSyncedSkills: Array; - seenUpstreamIds: Set; + excludedUpstreamIds: Set; }): (ISkill & { _id: Types.ObjectId }) | null { const sourceTenantId = params.source.tenantId ?? undefined; const sourceAuthor = params.prepared.createInput.author.toString(); @@ -1129,7 +1129,7 @@ function findMovedSourceSkill(params: { if (!upstreamId) { return false; } - return !params.seenUpstreamIds.has(upstreamId); + return !params.excludedUpstreamIds.has(upstreamId); }) ?? null ); } @@ -1145,7 +1145,7 @@ function hasNameConflictingStaleSkill(params: { source: params.source, prepared: params.prepared.prepared, existingSyncedSkills: params.existingSyncedSkills, - seenUpstreamIds: params.discoveredUpstreamIds, + excludedUpstreamIds: params.discoveredUpstreamIds, }), ); } @@ -1223,7 +1223,7 @@ async function deleteNameConflictingStaleSkill(params: { source: params.source, prepared: params.prepared, existingSyncedSkills: params.existingSyncedSkills, - seenUpstreamIds: params.discoveredUpstreamIds, + excludedUpstreamIds: params.discoveredUpstreamIds, }); if (!staleSkill) { return { @@ -1493,7 +1493,7 @@ async function syncSource(params: { source, prepared, existingSyncedSkills: await getExistingSyncedSkills(), - seenUpstreamIds, + excludedUpstreamIds: discoveredUpstreamIds, }); const effectivePrepared: PreparedRemoteSkill = movedExisting ? { ...prepared, existing: movedExisting } diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index 6638330bd9..231ba280d3 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -216,6 +216,23 @@ describe('createSkillSyncTriggerOrchestrator', () => { ); }); + it('preserves configured tenant scope for platform admin override runners', async () => { + const config = skillSync(); + const { orchestrator, runners } = createHarness(); + + orchestrator.getRunnerForAdminRequest({ + config: { skillSync: config, config: {} }, + user: {}, + skillSyncAllowServerCredentials: true, + }); + const runnerConfig = await runners[0].input.getConfig(); + + expect(runnerConfig?.github?.runOnStartup).toBe(true); + expect(runnerConfig?.github?.sources[0]).toEqual( + expect.objectContaining({ id: 'tenant-skills', tenantId: 'other-tenant' }), + ); + }); + it('preserves configured tenant scope for admin base skillSync runs', async () => { const config = skillSync(); const { orchestrator, runners } = createHarness(); diff --git a/packages/api/src/skills/sync/orchestrator.ts b/packages/api/src/skills/sync/orchestrator.ts index a4b14d261a..2783787ff7 100644 --- a/packages/api/src/skills/sync/orchestrator.ts +++ b/packages/api/src/skills/sync/orchestrator.ts @@ -96,7 +96,9 @@ function withRequestTenant( ...(disableRunOnStartup ? { runOnStartup: false } : {}), sources: config.github.sources.map((source) => ({ ...source, - tenantId, + // Tenant-scoped requests derive their tenant from the request; platform + // requests have no tenant and preserve an explicitly configured source. + tenantId: tenantId ?? source.tenantId, })), }, };