From d79712bf5d534d2acbd8854be43ceac17283d24d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 1 Jun 2026 19:09:29 -0400 Subject: [PATCH] fix: Guard existing skill sync permission grants --- packages/api/src/skills/sync/github.spec.ts | 38 +++++++++++++++++++++ packages/api/src/skills/sync/github.ts | 5 ++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 46f7bccb11..029942c2fc 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -1069,6 +1069,44 @@ describe('createGitHubSkillSyncRunner', () => { expect(deps.grantPermission).toHaveBeenCalled(); }); + it('does not mutate existing skill files when permission grant fails', async () => { + const existing = makeSkill({ + name: 'research', + description: 'Old description', + body: 'Old body', + author: new Types.ObjectId(), + 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({ + findSkillBySourceIdentity: jest.fn(async () => existing), + getSkillById: jest.fn(async () => existing), + grantPermission: jest.fn(async () => { + throw new Error('permission unavailable'); + }), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.grantPermission).toHaveBeenCalledWith( + expect.objectContaining({ resourceId: existing._id }), + ); + expect(deps.listSkillFiles).not.toHaveBeenCalled(); + expect(deps.saveBuffer).not.toHaveBeenCalled(); + expect(deps.upsertSkillFile).not.toHaveBeenCalled(); + expect(deps.updateSkill).not.toHaveBeenCalled(); + }); + it('restores existing skill files when the skill update fails after file sync', 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 63f6967b3d..f71212f31c 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -1276,10 +1276,10 @@ async function syncSource(params: { `Skill "${effectivePrepared.existing.name}" was modified during sync`, ); } + await ensurePublicViewer(deps, effectivePrepared.existing._id); const previousFiles = await deps.listSkillFiles(effectivePrepared.existing._id); const journal: SyncSkillFilesJournal = { staleFiles: [], savedFiles: [] }; let fileCounts: SyncSkillFilesResult; - let upserted: UpsertRemoteSkillResult; try { fileCounts = await syncSkillFiles({ deps, @@ -1292,7 +1292,7 @@ async function syncSource(params: { assertNotCancelled, journal, }); - upserted = await commitExistingRemoteSkillAfterFileSync( + await commitExistingRemoteSkillAfterFileSync( deps, { ...effectivePrepared, @@ -1314,7 +1314,6 @@ async function syncSource(params: { ); throw error; } - await ensurePublicViewer(deps, upserted.skill._id); await cleanupStoredFiles({ deps, files: fileCounts.staleFiles,