mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-11 08:43:48 +00:00
fix: Harden GitHub skill sync review paths
This commit is contained in:
Danny Avila
parent
766368f513
commit
7c7e27a6b8
5 changed files with 387 additions and 55 deletions
|
|
@ -23,8 +23,10 @@ const {
|
|||
} = require('@librechat/api');
|
||||
const { connectDb, indexSync } = require('~/db');
|
||||
const initializeOAuthReconnectManager = require('./services/initializeOAuthReconnectManager');
|
||||
const { capabilityContextMiddleware } = require('./middleware/roles/capabilities');
|
||||
const createValidateImageRequest = require('./middleware/validateImageRequest');
|
||||
const { startExpiredFileSweep } = require('./services/Files/process');
|
||||
const { initializeGitHubSkillSync } = require('./services/Skills/sync');
|
||||
const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies');
|
||||
const { updateInterfacePermissions: updateInterfacePerms } = require('@librechat/api');
|
||||
const {
|
||||
|
|
@ -294,6 +296,7 @@ if (cluster.isMaster) {
|
|||
/** Initialize app configuration */
|
||||
const appConfig = await getAppConfig();
|
||||
initializeFileStorage(appConfig);
|
||||
initializeGitHubSkillSync(appConfig);
|
||||
expiredFileSweepOptions = { appConfig, loadAppConfig: getAppConfig };
|
||||
startExpiredFileSweepOnce();
|
||||
await performStartupChecks(appConfig);
|
||||
|
|
@ -370,10 +373,13 @@ if (cluster.isMaster) {
|
|||
await configureSocialLogins(app);
|
||||
}
|
||||
|
||||
app.use(capabilityContextMiddleware);
|
||||
|
||||
/** Routes */
|
||||
app.use('/oauth', routes.oauth);
|
||||
app.use('/api/auth', routes.auth);
|
||||
app.use('/api/admin', routes.adminAuth);
|
||||
app.use('/api/admin/skills', routes.adminSkills);
|
||||
app.use('/api/actions', routes.actions);
|
||||
app.use('/api/keys', routes.keys);
|
||||
app.use('/api/api-keys', routes.apiKeys);
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ const { getGitHubSkillSyncRunner } = require('~/server/services/Skills/sync');
|
|||
|
||||
const router = express.Router();
|
||||
const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN);
|
||||
const requireManageSkills = requireCapability(SystemCapabilities.MANAGE_SKILLS);
|
||||
|
||||
const handlers = createAdminSkillsSyncHandlers({
|
||||
runner: getGitHubSkillSyncRunner(),
|
||||
|
|
@ -18,8 +19,8 @@ const handlers = createAdminSkillsSyncHandlers({
|
|||
router.use(requireJwtAuth, requireAdminAccess);
|
||||
|
||||
router.get('/sync/status', handlers.getSyncStatus);
|
||||
router.post('/sync/run', handlers.runSync);
|
||||
router.put('/sync/credentials/:credentialKey', handlers.setCredential);
|
||||
router.delete('/sync/credentials/:credentialKey', handlers.deleteCredential);
|
||||
router.post('/sync/run', requireManageSkills, handlers.runSync);
|
||||
router.put('/sync/credentials/:credentialKey', requireManageSkills, handlers.setCredential);
|
||||
router.delete('/sync/credentials/:credentialKey', requireManageSkills, handlers.deleteCredential);
|
||||
|
||||
module.exports = router;
|
||||
|
|
|
|||
|
|
@ -2,11 +2,11 @@ const express = require('express');
|
|||
const request = require('supertest');
|
||||
|
||||
const mockRequireJwtAuth = jest.fn((req, res, next) => next());
|
||||
const mockRequireAdminAccess = jest.fn((req, res, next) => next());
|
||||
const mockRequireCapability = jest.fn(() => mockRequireAdminAccess);
|
||||
const mockCapabilityMiddleware = jest.fn((req, res, next) => next());
|
||||
const mockRequireCapability = jest.fn(() => mockCapabilityMiddleware);
|
||||
|
||||
jest.mock('@librechat/data-schemas', () => ({
|
||||
SystemCapabilities: { ACCESS_ADMIN: 'ACCESS_ADMIN' },
|
||||
SystemCapabilities: { ACCESS_ADMIN: 'access:admin', MANAGE_SKILLS: 'manage:skills' },
|
||||
}));
|
||||
|
||||
jest.mock('@librechat/api', () => ({
|
||||
|
|
@ -39,16 +39,20 @@ jest.mock('~/server/services/Skills/sync', () => ({
|
|||
}));
|
||||
|
||||
describe('admin skills sync routes', () => {
|
||||
it('requires JWT auth and ACCESS_ADMIN for sync endpoints', async () => {
|
||||
it('requires JWT auth and admin capabilities for sync endpoints', async () => {
|
||||
const router = require('./skills');
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use('/api/admin/skills', router);
|
||||
|
||||
await request(app).get('/api/admin/skills/sync/status').expect(200);
|
||||
await request(app).post('/api/admin/skills/sync/run').expect(200);
|
||||
await request(app).put('/api/admin/skills/sync/credentials/default').send({}).expect(200);
|
||||
await request(app).delete('/api/admin/skills/sync/credentials/default').expect(200);
|
||||
|
||||
expect(mockRequireCapability).toHaveBeenCalledWith('ACCESS_ADMIN');
|
||||
expect(mockRequireCapability).toHaveBeenCalledWith('access:admin');
|
||||
expect(mockRequireCapability).toHaveBeenCalledWith('manage:skills');
|
||||
expect(mockRequireJwtAuth).toHaveBeenCalled();
|
||||
expect(mockRequireAdminAccess).toHaveBeenCalled();
|
||||
expect(mockCapabilityMiddleware).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -109,6 +109,35 @@ function makeSkill(input: CreateSkillInput): ISkill & { _id: Types.ObjectId } {
|
|||
};
|
||||
}
|
||||
|
||||
function makeSkillFile(
|
||||
skill: ISkill & { _id: Types.ObjectId },
|
||||
overrides: Partial<ISkillFile> = {},
|
||||
): ISkillFile & { _id: Types.ObjectId } {
|
||||
return {
|
||||
_id: new Types.ObjectId(),
|
||||
skillId: skill._id,
|
||||
relativePath: 'scripts/run.sh',
|
||||
file_id: 'old-file-id',
|
||||
filename: 'run.sh',
|
||||
filepath: '/uploads/old-file-id__run.sh',
|
||||
source: 'local',
|
||||
sourceMetadata: {
|
||||
provider: 'github',
|
||||
sourceId: 'librechat-skills',
|
||||
upstreamId: 'librechat-skills:skills/research',
|
||||
commitSha: 'old-commit-sha',
|
||||
blobSha: 'old-file-sha',
|
||||
path: 'skills/research/scripts/run.sh',
|
||||
},
|
||||
mimeType: 'application/x-sh',
|
||||
bytes: 7,
|
||||
category: 'script',
|
||||
isExecutable: false,
|
||||
author: skill.author,
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
function createDeps(
|
||||
overrides: Partial<GitHubSkillSyncDeps> = {},
|
||||
): GitHubSkillSyncDeps & { statuses: ISkillSyncStatus[] } {
|
||||
|
|
@ -657,6 +686,146 @@ describe('createGitHubSkillSyncRunner', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('skips existing skill updates when the upstream package is unchanged', async () => {
|
||||
const skillMarkdown = '---\nname: research\ndescription: Research things\n---\nBody';
|
||||
const existing = makeSkill({
|
||||
name: 'research',
|
||||
description: 'Research things',
|
||||
body: skillMarkdown,
|
||||
frontmatter: {},
|
||||
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',
|
||||
commitSha: 'old-commit-sha',
|
||||
skillBlobSha: 'skill-md-sha',
|
||||
syncedAt: '2026-05-30T00:00:00.000Z',
|
||||
syncStatus: 'synced',
|
||||
},
|
||||
}) as ISkill & { _id: Types.ObjectId };
|
||||
const unchangedFile = makeSkillFile(existing, {
|
||||
sourceMetadata: {
|
||||
provider: 'github',
|
||||
sourceId: 'librechat-skills',
|
||||
upstreamId: 'librechat-skills:skills/research',
|
||||
commitSha: 'old-commit-sha',
|
||||
blobSha: 'file-sha',
|
||||
path: 'skills/research/scripts/run.sh',
|
||||
},
|
||||
});
|
||||
const deps = createDeps({
|
||||
fetchFn: githubFetch(skillMarkdown),
|
||||
findSkillBySourceIdentity: jest.fn(async () => existing),
|
||||
getSkillById: jest.fn(async () => existing),
|
||||
getSkillFileByPath: jest.fn(async () => unchangedFile),
|
||||
listSkillFiles: jest.fn(async () => [unchangedFile]),
|
||||
updateSkill: jest.fn(),
|
||||
});
|
||||
const runner = createGitHubSkillSyncRunner(deps);
|
||||
const result = await runner.runOnce();
|
||||
|
||||
expect(result.status).toBe('completed');
|
||||
expect(deps.saveBuffer).not.toHaveBeenCalled();
|
||||
expect(deps.upsertSkillFile).not.toHaveBeenCalled();
|
||||
expect(deps.updateSkill).not.toHaveBeenCalled();
|
||||
expect(deps.grantPermission).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('restores existing skill files when the skill update fails after file sync', 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 oldFile = makeSkillFile(existing);
|
||||
const files = new Map<string, ISkillFile & { _id: Types.ObjectId }>([
|
||||
[oldFile.relativePath, oldFile],
|
||||
]);
|
||||
const upsertSkillFile = jest.fn(
|
||||
async (
|
||||
row: Parameters<GitHubSkillSyncDeps['upsertSkillFile']>[0],
|
||||
): Promise<ISkillFile & { _id: Types.ObjectId }> => {
|
||||
const current = files.get(row.relativePath);
|
||||
const next = {
|
||||
_id: current?._id ?? new Types.ObjectId(),
|
||||
skillId: row.skillId as Types.ObjectId,
|
||||
relativePath: row.relativePath,
|
||||
file_id: row.file_id,
|
||||
filename: row.filename,
|
||||
filepath: row.filepath,
|
||||
storageKey: row.storageKey,
|
||||
storageRegion: row.storageRegion,
|
||||
source: row.source,
|
||||
sourceMetadata: row.sourceMetadata,
|
||||
mimeType: row.mimeType,
|
||||
bytes: row.bytes,
|
||||
category: 'script' as const,
|
||||
isExecutable: row.isExecutable ?? false,
|
||||
author: row.author,
|
||||
tenantId: row.tenantId,
|
||||
};
|
||||
files.set(row.relativePath, next);
|
||||
return next;
|
||||
},
|
||||
);
|
||||
const deps = createDeps({
|
||||
findSkillBySourceIdentity: jest.fn(async () => existing),
|
||||
getSkillById: jest.fn(async () => ({ ...existing, version: existing.version + 1 })),
|
||||
getSkillFileByPath: jest.fn(
|
||||
async (_skillId, relativePath) => files.get(relativePath) ?? null,
|
||||
),
|
||||
listSkillFiles: jest.fn(async () => Array.from(files.values())),
|
||||
upsertSkillFile,
|
||||
deleteSkillFile: jest.fn(async (_skillId, relativePath) => ({
|
||||
deleted: files.delete(relativePath),
|
||||
})),
|
||||
saveBuffer: jest.fn(async () => ({
|
||||
filepath: '/uploads/new-file-id__run.sh',
|
||||
source: 'local',
|
||||
})),
|
||||
deleteFile: jest.fn(async () => undefined),
|
||||
updateSkill: jest.fn(async () => ({ status: 'conflict' as const })),
|
||||
});
|
||||
const runner = createGitHubSkillSyncRunner(deps);
|
||||
const result = await runner.runOnce();
|
||||
|
||||
expect(result.status).toBe('failed');
|
||||
expect(deps.upsertStatus).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({
|
||||
status: 'failed',
|
||||
errorCode: 'SKILL_CONFLICT',
|
||||
}),
|
||||
);
|
||||
expect(files.get('scripts/run.sh')).toEqual(
|
||||
expect.objectContaining({ filepath: oldFile.filepath }),
|
||||
);
|
||||
expect(deps.deleteFile).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ filepath: '/uploads/new-file-id__run.sh' }),
|
||||
);
|
||||
expect(deps.deleteFile).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ filepath: oldFile.filepath }),
|
||||
);
|
||||
});
|
||||
|
||||
it('preserves credential presence when a manual run is skipped by an active lock', async () => {
|
||||
const deps = createDeps({
|
||||
tryAcquireLock: jest.fn(async () => false),
|
||||
|
|
@ -698,6 +867,26 @@ describe('createGitHubSkillSyncRunner', () => {
|
|||
]);
|
||||
});
|
||||
|
||||
it('uses a fresh lock owner for each sync run', async () => {
|
||||
const deps = createDeps({ lockOwner: 'worker-a' });
|
||||
const runner = createGitHubSkillSyncRunner(deps);
|
||||
|
||||
await runner.runOnce();
|
||||
await runner.runOnce();
|
||||
|
||||
const lockOwners = (deps.tryAcquireLock as jest.Mock).mock.calls.map(
|
||||
([params]: [Parameters<GitHubSkillSyncDeps['tryAcquireLock']>[0]]) => params.lockOwner,
|
||||
);
|
||||
const releasedOwners = (deps.releaseLock as jest.Mock).mock.calls.map(
|
||||
([params]: [Parameters<GitHubSkillSyncDeps['releaseLock']>[0]]) => params.lockOwner,
|
||||
);
|
||||
|
||||
expect(lockOwners).toHaveLength(2);
|
||||
expect(lockOwners[0]).not.toBe(lockOwners[1]);
|
||||
expect(lockOwners.every((owner) => owner.startsWith('worker-a:'))).toBe(true);
|
||||
expect(releasedOwners).toEqual(lockOwners);
|
||||
});
|
||||
|
||||
it('excludes child skill packages from parent synced files', async () => {
|
||||
const parentSkillMarkdown = '---\nname: parent\ndescription: Parent skill\n---\nParent';
|
||||
const childSkillMarkdown = '---\nname: child\ndescription: Child skill\n---\nChild';
|
||||
|
|
|
|||
|
|
@ -99,6 +99,23 @@ type SaveBufferResult = {
|
|||
storageRegion?: string;
|
||||
};
|
||||
|
||||
type StoredSkillFileRef = {
|
||||
filepath: string;
|
||||
source: string;
|
||||
storageKey?: string;
|
||||
storageRegion?: string;
|
||||
author?: Types.ObjectId | string;
|
||||
tenantId?: string;
|
||||
};
|
||||
|
||||
type SyncSkillFilesJournal = {
|
||||
staleFiles: StoredSkillFileRef[];
|
||||
savedFiles: StoredSkillFileRef[];
|
||||
};
|
||||
|
||||
type SyncSkillFilesResult = Pick<SyncCounters, 'syncedFileCount' | 'deletedFileCount'> &
|
||||
SyncSkillFilesJournal;
|
||||
|
||||
type MaybePromise<T> = T | Promise<T>;
|
||||
|
||||
export type GitHubSkillSyncDeps = {
|
||||
|
|
@ -828,6 +845,7 @@ function hasExternalSkillEdit(before: ISkill, after: ISkill): boolean {
|
|||
async function commitExistingRemoteSkillAfterFileSync(
|
||||
deps: GitHubSkillSyncDeps,
|
||||
prepared: PreparedExistingRemoteSkill,
|
||||
options: { forceCommit?: boolean } = {},
|
||||
): Promise<UpsertRemoteSkillResult> {
|
||||
const refreshed = await deps.getSkillById(prepared.existing._id);
|
||||
if (!refreshed) {
|
||||
|
|
@ -842,13 +860,13 @@ async function commitExistingRemoteSkillAfterFileSync(
|
|||
`Skill "${prepared.existing.name}" was modified during sync`,
|
||||
);
|
||||
}
|
||||
if (!options.forceCommit && !hasRemoteSkillDefinitionChanged(prepared.update, refreshed)) {
|
||||
return { skill: refreshed, created: false };
|
||||
}
|
||||
return commitRemoteSkill(deps, { ...prepared, existing: refreshed });
|
||||
}
|
||||
|
||||
async function cleanupFile(
|
||||
deps: GitHubSkillSyncDeps,
|
||||
file: ISkillFile & { _id: Types.ObjectId },
|
||||
): Promise<void> {
|
||||
async function cleanupFile(deps: GitHubSkillSyncDeps, file: StoredSkillFileRef): Promise<void> {
|
||||
if (!deps.deleteFile) {
|
||||
return;
|
||||
}
|
||||
|
|
@ -862,6 +880,105 @@ async function cleanupFile(
|
|||
});
|
||||
}
|
||||
|
||||
function toStoredFileRef(params: {
|
||||
saved: SaveBufferResult;
|
||||
author: Types.ObjectId;
|
||||
tenantId?: string;
|
||||
}): StoredSkillFileRef {
|
||||
return {
|
||||
filepath: params.saved.filepath,
|
||||
source: params.saved.source,
|
||||
storageKey: params.saved.storageKey,
|
||||
storageRegion: params.saved.storageRegion,
|
||||
author: params.author,
|
||||
tenantId: params.tenantId,
|
||||
};
|
||||
}
|
||||
|
||||
function toSkillFileInput(file: ISkillFile & { _id: Types.ObjectId }): UpsertSkillFileInput {
|
||||
return {
|
||||
skillId: file.skillId,
|
||||
relativePath: file.relativePath,
|
||||
file_id: file.file_id,
|
||||
filename: file.filename,
|
||||
filepath: file.filepath,
|
||||
storageKey: file.storageKey,
|
||||
storageRegion: file.storageRegion,
|
||||
source: file.source,
|
||||
sourceMetadata: file.sourceMetadata,
|
||||
mimeType: file.mimeType,
|
||||
bytes: file.bytes,
|
||||
isExecutable: file.isExecutable,
|
||||
author: file.author,
|
||||
tenantId: file.tenantId,
|
||||
};
|
||||
}
|
||||
|
||||
function getStoredFileKey(file: StoredSkillFileRef): string {
|
||||
return [file.source, file.filepath, file.storageKey ?? '', file.storageRegion ?? ''].join(':');
|
||||
}
|
||||
|
||||
async function cleanupStoredFiles(params: {
|
||||
deps: GitHubSkillSyncDeps;
|
||||
files: StoredSkillFileRef[];
|
||||
logMessage: string;
|
||||
}): Promise<void> {
|
||||
const seen = new Set<string>();
|
||||
for (const file of params.files) {
|
||||
const key = getStoredFileKey(file);
|
||||
if (seen.has(key)) {
|
||||
continue;
|
||||
}
|
||||
seen.add(key);
|
||||
await cleanupFile(params.deps, file).catch((cleanupError) =>
|
||||
logger.error(params.logMessage, cleanupError),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async function restoreExistingSkillFiles(params: {
|
||||
deps: GitHubSkillSyncDeps;
|
||||
skill: ISkill & { _id: Types.ObjectId };
|
||||
previousFiles: Array<ISkillFile & { _id: Types.ObjectId }>;
|
||||
savedFiles: StoredSkillFileRef[];
|
||||
}): Promise<void> {
|
||||
const { deps, skill, previousFiles, savedFiles } = params;
|
||||
const previousByPath = new Map(previousFiles.map((file) => [file.relativePath, file]));
|
||||
const currentFiles = await deps.listSkillFiles(skill._id);
|
||||
|
||||
for (const file of currentFiles) {
|
||||
if (previousByPath.has(file.relativePath)) {
|
||||
continue;
|
||||
}
|
||||
await deps.deleteSkillFile(skill._id, file.relativePath);
|
||||
}
|
||||
for (const file of previousFiles) {
|
||||
await deps.upsertSkillFile(toSkillFileInput(file));
|
||||
}
|
||||
await cleanupStoredFiles({
|
||||
deps,
|
||||
files: savedFiles,
|
||||
logMessage: '[GitHubSkillSync] Failed to clean up rolled-back synced file:',
|
||||
});
|
||||
}
|
||||
|
||||
function comparableSourceMetadata(metadata: Record<string, unknown> | undefined): string {
|
||||
const { commitSha: _commitSha, syncedAt: _syncedAt, ...rest } = metadata ?? {};
|
||||
return JSON.stringify(rest);
|
||||
}
|
||||
|
||||
function hasRemoteSkillDefinitionChanged(update: UpdateSkillInput, existing: ISkill): boolean {
|
||||
return (
|
||||
update.body !== existing.body ||
|
||||
update.name !== existing.name ||
|
||||
update.description !== existing.description ||
|
||||
(update.alwaysApply ?? false) !== (existing.alwaysApply ?? false) ||
|
||||
JSON.stringify(update.frontmatter ?? {}) !== JSON.stringify(existing.frontmatter ?? {}) ||
|
||||
comparableSourceMetadata(update.sourceMetadata) !==
|
||||
comparableSourceMetadata(existing.sourceMetadata)
|
||||
);
|
||||
}
|
||||
|
||||
async function syncSkillFiles(params: {
|
||||
deps: GitHubSkillSyncDeps;
|
||||
token: string;
|
||||
|
|
@ -871,8 +988,10 @@ async function syncSkillFiles(params: {
|
|||
commitSha: string;
|
||||
fetchFn: FetchFn;
|
||||
assertNotCancelled: AssertNotCancelled;
|
||||
}): Promise<Pick<SyncCounters, 'syncedFileCount' | 'deletedFileCount'>> {
|
||||
journal?: SyncSkillFilesJournal;
|
||||
}): Promise<SyncSkillFilesResult> {
|
||||
const { deps, token, source, skill, discovered, commitSha, fetchFn, assertNotCancelled } = params;
|
||||
const journal = params.journal ?? { staleFiles: [], savedFiles: [] };
|
||||
const remotePaths = new Set<string>();
|
||||
let syncedFileCount = 0;
|
||||
let deletedFileCount = 0;
|
||||
|
|
@ -887,7 +1006,6 @@ async function syncSkillFiles(params: {
|
|||
totalFileBytes += assertGitHubBlobSize(entry, relativePath);
|
||||
assertCumulativeGitHubFileSize(totalFileBytes);
|
||||
remotePaths.add(relativePath);
|
||||
syncedFileCount++;
|
||||
const existing = await deps.getSkillFileByPath(skill._id, relativePath);
|
||||
if (existing && getSourceMetadataString(existing, 'blobSha') === entry.sha) {
|
||||
continue;
|
||||
|
|
@ -906,6 +1024,7 @@ async function syncSkillFiles(params: {
|
|||
isImage: mimeType.startsWith('image/'),
|
||||
tenantId: skill.tenantId,
|
||||
});
|
||||
const savedFile = toStoredFileRef({ saved, author: skill.author, tenantId: skill.tenantId });
|
||||
try {
|
||||
await deps.upsertSkillFile({
|
||||
skillId: skill._id,
|
||||
|
|
@ -931,29 +1050,15 @@ async function syncSkillFiles(params: {
|
|||
tenantId: skill.tenantId,
|
||||
});
|
||||
} catch (error) {
|
||||
if (deps.deleteFile) {
|
||||
await deps
|
||||
.deleteFile({
|
||||
filepath: saved.filepath,
|
||||
source: saved.source,
|
||||
storageKey: saved.storageKey,
|
||||
storageRegion: saved.storageRegion,
|
||||
user: skill.author,
|
||||
tenantId: skill.tenantId,
|
||||
})
|
||||
.catch((cleanupError) =>
|
||||
logger.error(
|
||||
'[GitHubSkillSync] Failed to clean up orphaned synced file:',
|
||||
cleanupError,
|
||||
),
|
||||
);
|
||||
}
|
||||
await cleanupFile(deps, savedFile).catch((cleanupError) =>
|
||||
logger.error('[GitHubSkillSync] Failed to clean up orphaned synced file:', cleanupError),
|
||||
);
|
||||
throw error;
|
||||
}
|
||||
syncedFileCount++;
|
||||
journal.savedFiles.push(savedFile);
|
||||
if (existing && existing.filepath !== saved.filepath) {
|
||||
await cleanupFile(deps, existing).catch((cleanupError) =>
|
||||
logger.error('[GitHubSkillSync] Failed to clean up replaced synced file:', cleanupError),
|
||||
);
|
||||
journal.staleFiles.push(existing);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -963,15 +1068,13 @@ async function syncSkillFiles(params: {
|
|||
if (remotePaths.has(file.relativePath)) {
|
||||
continue;
|
||||
}
|
||||
await cleanupFile(deps, file).catch((cleanupError) =>
|
||||
logger.error('[GitHubSkillSync] Failed to clean up deleted synced file:', cleanupError),
|
||||
);
|
||||
const result = await deps.deleteSkillFile(skill._id, file.relativePath);
|
||||
if (result.deleted) {
|
||||
deletedFileCount++;
|
||||
journal.staleFiles.push(file);
|
||||
}
|
||||
}
|
||||
return { syncedFileCount, deletedFileCount };
|
||||
return { syncedFileCount, deletedFileCount, ...journal };
|
||||
}
|
||||
|
||||
async function deleteSyncedSkill(
|
||||
|
|
@ -1068,21 +1171,50 @@ async function syncSource(params: {
|
|||
`Skill "${prepared.existing.name}" was modified during sync`,
|
||||
);
|
||||
}
|
||||
const fileCounts = await syncSkillFiles({
|
||||
deps,
|
||||
token,
|
||||
source,
|
||||
skill: prepared.existing,
|
||||
discovered,
|
||||
commitSha: commit.sha,
|
||||
fetchFn,
|
||||
assertNotCancelled,
|
||||
});
|
||||
const upserted = await commitExistingRemoteSkillAfterFileSync(deps, {
|
||||
...prepared,
|
||||
existing: prepared.existing,
|
||||
});
|
||||
const previousFiles = await deps.listSkillFiles(prepared.existing._id);
|
||||
const journal: SyncSkillFilesJournal = { staleFiles: [], savedFiles: [] };
|
||||
let fileCounts: SyncSkillFilesResult;
|
||||
let upserted: UpsertRemoteSkillResult;
|
||||
try {
|
||||
fileCounts = await syncSkillFiles({
|
||||
deps,
|
||||
token,
|
||||
source,
|
||||
skill: prepared.existing,
|
||||
discovered,
|
||||
commitSha: commit.sha,
|
||||
fetchFn,
|
||||
assertNotCancelled,
|
||||
journal,
|
||||
});
|
||||
upserted = await commitExistingRemoteSkillAfterFileSync(
|
||||
deps,
|
||||
{
|
||||
...prepared,
|
||||
existing: prepared.existing,
|
||||
},
|
||||
{ forceCommit: fileCounts.syncedFileCount > 0 || fileCounts.deletedFileCount > 0 },
|
||||
);
|
||||
} catch (error) {
|
||||
await restoreExistingSkillFiles({
|
||||
deps,
|
||||
skill: prepared.existing,
|
||||
previousFiles,
|
||||
savedFiles: journal.savedFiles,
|
||||
}).catch((cleanupError) =>
|
||||
logger.error(
|
||||
'[GitHubSkillSync] Failed to restore existing skill files after sync failure:',
|
||||
cleanupError,
|
||||
),
|
||||
);
|
||||
throw error;
|
||||
}
|
||||
await ensurePublicViewer(deps, upserted.skill._id);
|
||||
await cleanupStoredFiles({
|
||||
deps,
|
||||
files: fileCounts.staleFiles,
|
||||
logMessage: '[GitHubSkillSync] Failed to clean up replaced synced file:',
|
||||
});
|
||||
counts.syncedSkillCount++;
|
||||
counts.syncedFileCount += fileCounts.syncedFileCount;
|
||||
counts.deletedFileCount += fileCounts.deletedFileCount;
|
||||
|
|
@ -1206,8 +1338,7 @@ function getGithubConfig(config: SkillSyncConfig | undefined): {
|
|||
|
||||
export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
|
||||
const fetchFn = deps.fetchFn ?? fetch;
|
||||
const lockOwner =
|
||||
deps.lockOwner ?? `${process.pid}:${crypto.randomUUID().replace(/-/g, '').slice(0, 12)}`;
|
||||
const lockOwnerPrefix = deps.lockOwner ?? `${process.pid}`;
|
||||
|
||||
async function getStatus() {
|
||||
const github = getGithubConfig(await deps.getConfig());
|
||||
|
|
@ -1261,6 +1392,7 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
|
|||
if (!github.enabled || github.sources.length === 0) {
|
||||
return { status: 'skipped', message: 'GitHub skill sync is disabled', sources: [] };
|
||||
}
|
||||
const lockOwner = `${lockOwnerPrefix}:${crypto.randomUUID().replace(/-/g, '').slice(0, 12)}`;
|
||||
const acquired = await deps.tryAcquireLock({
|
||||
provider: PROVIDER,
|
||||
lockOwner,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue