From 7fedb5c027ba5a5850d55e9ce8d9a6ae37cda126 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 30 May 2026 13:15:56 -0400 Subject: [PATCH] fix: Repair tsc error and guard external edits in github skill sync - Fix TS2352 in github.spec mirror-cleanup test: build the existing-skill mock via makeSkill with authorName instead of an under-typed 'as CreateSkillInput' cast (this was the failing TypeScript CI check on f00ce3c5a). - 808: commitExistingRemoteSkillAfterFileSync re-reads to clear our own file-sync version bumps, but now compares refreshed content against the pre-sync snapshot (body/name/description/always-apply) and throws SKILL_CONFLICT on a concurrent external edit instead of overwriting it. --- packages/api/src/skills/sync/github.spec.ts | 23 +++++++++++---------- packages/api/src/skills/sync/github.ts | 22 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index e0acc27305..ed2f41d342 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -308,17 +308,18 @@ describe('createGitHubSkillSyncRunner', () => { it('scopes mirror cleanup to the current source and deletes only its absent upstream skills', async () => { const keptId = new Types.ObjectId(); const staleId = new Types.ObjectId(); - const existingSkill = (upstreamId: string, _id: Types.ObjectId) => - ({ - ...makeSkill({ - name: 'research', - description: 'Research things', - author: new Types.ObjectId(), - source: 'github', - sourceMetadata: { provider: 'github', sourceId: 'librechat-skills', upstreamId }, - } as CreateSkillInput), - _id, - }) as ISkill & { _id: Types.ObjectId }; + const existingSkill = (upstreamId: string, _id: Types.ObjectId) => { + const skill = makeSkill({ + name: 'research', + description: 'Research things', + author: new Types.ObjectId(), + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { provider: 'github', sourceId: 'librechat-skills', upstreamId }, + }); + skill._id = _id; + return skill; + }; const listSkillsBySource = jest.fn(async () => [ existingSkill('librechat-skills:LibreChat/skills:skills/research', keptId), existingSkill('librechat-skills:LibreChat/skills:skills/removed', staleId), diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 8ce41eed2a..af333c6be7 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -797,6 +797,22 @@ async function commitRemoteSkill( return { skill: created.skill, created: true }; } +/** + * File sync bumps the parent skill's `version` (via file upserts/deletes) but + * never changes its authored content, so we must re-read to get past our own + * version bumps. A plain re-read would also silently accept and overwrite a + * concurrent external edit; compare the refreshed content against the pre-sync + * snapshot and treat a changed body/name/description/always-apply as a conflict. + */ +function hasExternalSkillEdit(before: ISkill, after: ISkill): boolean { + return ( + before.body !== after.body || + before.name !== after.name || + before.description !== after.description || + (before.alwaysApply ?? false) !== (after.alwaysApply ?? false) + ); +} + async function commitExistingRemoteSkillAfterFileSync( deps: GitHubSkillSyncDeps, prepared: PreparedExistingRemoteSkill, @@ -808,6 +824,12 @@ async function commitExistingRemoteSkillAfterFileSync( `Previously synced skill "${prepared.existing.name}" was removed`, ); } + if (hasExternalSkillEdit(prepared.existing, refreshed)) { + throw new SkillSyncError( + 'SKILL_CONFLICT', + `Skill "${prepared.existing.name}" was modified during sync`, + ); + } return commitRemoteSkill(deps, { ...prepared, existing: refreshed }); }