fix: resolve codex skill sync edge findings

This commit is contained in:
Danny Avila 2026-06-08 00:01:36 -04:00
parent bd3c23fd13
commit d6d402dce1
4 changed files with 137 additions and 6 deletions

View file

@ -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',

View file

@ -1111,7 +1111,7 @@ function findMovedSourceSkill(params: {
source: SkillSyncGitHubSourceConfig;
prepared: PreparedRemoteSkill;
existingSyncedSkills: Array<ISkill & { _id: Types.ObjectId }>;
seenUpstreamIds: Set<string>;
excludedUpstreamIds: Set<string>;
}): (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 }

View file

@ -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();

View file

@ -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,
})),
},
};