diff --git a/api/server/routes/skills.test.js b/api/server/routes/skills.test.js index 11157e0ef7..55f227db5a 100644 --- a/api/server/routes/skills.test.js +++ b/api/server/routes/skills.test.js @@ -300,14 +300,27 @@ describe('Skill routes', () => { expect(res.status).toBe(400); }); - it('rejects frontmatter with unknown keys', async () => { + it('accepts frontmatter with unknown keys and warns about them', async () => { + const res = await createSkillAsOwner({ + name: 'unknown-key-frontmatter-skill', + frontmatter: { 'not-a-real-key': 'value' }, + }); + expect(res.status).toBe(201); + expect(res.body.warnings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ code: 'UNKNOWN_KEY', severity: 'warning' }), + ]), + ); + }); + + it('rejects malformed frontmatter with 400', async () => { const res = await createSkillAsOwner({ name: 'bad-frontmatter-skill', - frontmatter: { 'not-a-real-key': 'value' }, + frontmatter: { 'user-invocable': 'yes' }, }); expect(res.status).toBe(400); expect(res.body.issues).toEqual( - expect.arrayContaining([expect.objectContaining({ code: 'UNKNOWN_KEY' })]), + expect.arrayContaining([expect.objectContaining({ code: 'INVALID_TYPE' })]), ); }); diff --git a/packages/api/src/admin/skills.spec.ts b/packages/api/src/admin/skills.spec.ts index ce5e970d5b..ba847a77b4 100644 --- a/packages/api/src/admin/skills.spec.ts +++ b/packages/api/src/admin/skills.spec.ts @@ -36,6 +36,7 @@ function createSourceStatus(overrides: Partial = {}): SourceStatus syncedFileCount: 0, deletedSkillCount: 0, deletedFileCount: 0, + skippedSkillCount: 0, errorCode: undefined, errorMessage: undefined, startedAt: undefined, @@ -51,7 +52,14 @@ function createSourceStatus(overrides: Partial = {}): SourceStatus function createHandlers({ statusErrorCode, statusErrorMessage, -}: { statusErrorCode?: string; statusErrorMessage?: string } = {}) { + skippedSkillPath = 'skills/broken', + skippedSkillErrorMessage = 'skills/broken/SKILL.md: malformed frontmatter', +}: { + statusErrorCode?: string; + statusErrorMessage?: string; + skippedSkillPath?: string; + skippedSkillErrorMessage?: string; +} = {}) { const runner = { getStatus: jest.fn(async () => ({ enabled: true, @@ -59,6 +67,15 @@ function createHandlers({ runOnStartup: false, sources: [ createSourceStatus({ + skippedSkillCount: 1, + skippedSkills: [ + { + path: skippedSkillPath, + name: 'broken', + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: skippedSkillErrorMessage, + }, + ], errorCode: statusErrorCode, errorMessage: statusErrorMessage, }), @@ -77,9 +94,18 @@ function createHandlers({ status: 'completed' as const, sources: [ createSourceStatus({ - status: 'succeeded', + status: 'partial', syncedSkillCount: 1, syncedFileCount: 2, + skippedSkillCount: 1, + skippedSkills: [ + { + path: skippedSkillPath, + name: 'broken', + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: skippedSkillErrorMessage, + }, + ], errorCode: statusErrorCode, errorMessage: statusErrorMessage, }), @@ -119,6 +145,10 @@ describe('createAdminSkillsSyncHandlers', () => { repo: undefined, ref: undefined, paths: undefined, + /* Skipped entries name repository paths, so they are redacted with + the rest of the source metadata; the bare count is not. */ + skippedSkillCount: 1, + skippedSkills: undefined, }), ], }), @@ -233,6 +263,92 @@ describe('createAdminSkillsSyncHandlers', () => { ); }); + it('redacts promoted skipped-skill paths from tenant-scoped status reads', async () => { + const { handlers } = createHandlers({ + statusErrorCode: 'SKILL_PARSE_FAILED', + statusErrorMessage: 'skills/broken/SKILL.md: malformed frontmatter', + }); + const res = createResponse(); + + await handlers.getSyncStatus( + { + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncCanReadCredentials: false, + } as never, + res, + ); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + sources: [ + expect.objectContaining({ + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: 'One or more GitHub skills could not be synchronized', + skippedSkills: undefined, + }), + ], + }), + ); + }); + + it('does not mistake a promoted skipped-skill path for a credential failure', async () => { + const errorMessage = 'skills/credential-helper/SKILL.md: malformed frontmatter'; + const { handlers } = createHandlers({ + statusErrorCode: 'SKILL_PARSE_FAILED', + statusErrorMessage: errorMessage, + skippedSkillPath: 'skills/credential-helper', + skippedSkillErrorMessage: errorMessage, + }); + const res = createResponse(); + + await handlers.getSyncStatus( + { + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncCanReadCredentials: false, + } as never, + res, + ); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + sources: [ + expect.objectContaining({ + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: 'One or more GitHub skills could not be synchronized', + }), + ], + }), + ); + }); + + it('preserves a fatal source error that follows an earlier skipped skill', async () => { + const { handlers } = createHandlers({ + statusErrorCode: 'GITHUB_RATE_LIMITED', + statusErrorMessage: 'GitHub request failed with HTTP 403', + }); + const res = createResponse(); + + await handlers.getSyncStatus( + { + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncCanReadCredentials: false, + } as never, + res, + ); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + sources: [ + expect.objectContaining({ + errorCode: 'GITHUB_RATE_LIMITED', + errorMessage: 'GitHub request failed with HTTP 403', + skippedSkills: undefined, + }), + ], + }), + ); + }); + it('includes credential summaries and source credential metadata for platform status reads', async () => { const { handlers } = createHandlers({ statusErrorCode: 'MISSING_CREDENTIAL', @@ -254,6 +370,13 @@ describe('createAdminSkillsSyncHandlers', () => { ref: 'main', paths: ['skills'], errorMessage: 'Missing GitHub credential "github-skills-prod"', + skippedSkillCount: 1, + skippedSkills: [ + expect.objectContaining({ + path: 'skills/broken', + errorCode: 'SKILL_PARSE_FAILED', + }), + ], }), ], }), diff --git a/packages/api/src/admin/skills.ts b/packages/api/src/admin/skills.ts index 8d481047f8..50cb6d2f76 100644 --- a/packages/api/src/admin/skills.ts +++ b/packages/api/src/admin/skills.ts @@ -113,14 +113,29 @@ function isCredentialError(status: ISkillSyncStatus): boolean { ); } +function isPromotedSkippedSkillError(status: ISkillSyncStatus): boolean { + const firstSkippedSkill = status.skippedSkills?.[0]; + return Boolean( + firstSkippedSkill && + status.errorCode === firstSkippedSkill.errorCode && + status.errorMessage === firstSkippedSkill.errorMessage, + ); +} + function serializeErrorMessage( status: ISkillSyncStatus, { includeCredentialMetadata }: { includeCredentialMetadata: boolean }, ): string | undefined { - if (includeCredentialMetadata || !isCredentialError(status)) { + if (includeCredentialMetadata) { return status.errorMessage; } - return 'GitHub skill sync credentials are not available'; + if (isPromotedSkippedSkillError(status)) { + return 'One or more GitHub skills could not be synchronized'; + } + if (isCredentialError(status)) { + return 'GitHub skill sync credentials are not available'; + } + return status.errorMessage; } function serializeSourceStatus( @@ -149,6 +164,10 @@ function serializeSourceStatus( syncedFileCount: status.syncedFileCount, deletedSkillCount: status.deletedSkillCount, deletedFileCount: status.deletedFileCount, + skippedSkillCount: status.skippedSkillCount ?? 0, + /* The per-skill entries name repository paths, so they follow the same + visibility rule as owner/repo/paths rather than the bare count. */ + skippedSkills: includePrivateSourceMetadata ? status.skippedSkills : undefined, createdAt: toIso(status.createdAt), updatedAt: toIso(status.updatedAt), }; diff --git a/packages/api/src/agents/handlers.spec.ts b/packages/api/src/agents/handlers.spec.ts index 5239f7575b..c3a9da3298 100644 --- a/packages/api/src/agents/handlers.spec.ts +++ b/packages/api/src/agents/handlers.spec.ts @@ -1195,7 +1195,7 @@ describe('createToolExecuteHandler', () => { args: { path: 'skills/new-skill/SKILL.md', content: - '---\nname: new-skill\ndescription: Use for tests\ndisable-model-invocation: true\nallowed-tools:\n - execute_code\n---\n# New skill\n', + '---\nname: new-skill\ndescription: Use for tests\ndisable-model-invocation: true\nAllowed-Tools:\n - execute_code\n---\n# New skill\n', }, }, ]); @@ -1221,6 +1221,79 @@ describe('createToolExecuteHandler', () => { expect(grantSkillOwner).toHaveBeenCalledWith({ req, skillId: SKILL_ID }); }); + it('rejects case-colliding recognized frontmatter keys in create_file', async () => { + const createSkill = jest.fn(); + const handler = makeAuthoringHandler({ + getSkillByName: jest.fn(async () => null), + createSkill: createSkill as unknown as ToolExecuteOptions['createSkill'], + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_create_collision_skill', + name: 'create_file', + args: { + path: 'skills/collision-skill/SKILL.md', + content: + '---\nname: collision-skill\ndescription: Use for collision tests\nallowed-tools:\n - read_file\nAllowed-Tools:\n - execute_code\n---\n# Collision skill\n', + }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('both resolve to "allowed-tools"'); + expect(createSkill).not.toHaveBeenCalled(); + }); + + it('surfaces skill validation warnings from create_file', async () => { + const createSkill = jest.fn(async () => ({ + skill: { + _id: SKILL_ID, + name: 'warning-skill', + body: '# Warning skill', + version: 1, + }, + warnings: [ + { + field: 'frontmatter.triger', + code: 'UNKNOWN_KEY', + severity: 'warning' as const, + message: '"triger" is not a recognized frontmatter key and is stored as-is', + }, + ], + })); + const handler = makeAuthoringHandler({ + getSkillByName: jest.fn(async () => null), + createSkill, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_create_warning_skill', + name: 'create_file', + args: { + path: 'skills/warning-skill/SKILL.md', + content: + '---\nname: warning-skill\ndescription: Use for warning tests\ntriger: manual\n---\n# Warning skill\n', + }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).toContain('Warnings:'); + expect(result.content).toContain('frontmatter.triger [UNKNOWN_KEY]'); + expect(result.artifact).toMatchObject({ + warning_count: 1, + warnings: [ + expect.objectContaining({ + field: 'frontmatter.triger', + code: 'UNKNOWN_KEY', + severity: 'warning', + }), + ], + }); + }); + it('adds required SKILL.md frontmatter when create_file only provides markdown', async () => { const createSkill = jest.fn(async () => ({ skill: { @@ -1871,6 +1944,65 @@ describe('createToolExecuteHandler', () => { ); }); + it('surfaces skill validation warnings from edit_file', async () => { + const oldBody = '---\nname: runtime-skill\ndescription: Use before\n---\n# Runtime skill\n'; + const updatedBody = + '---\nname: runtime-skill\ndescription: Use after\ntriger: manual\n---\n# Runtime skill\n'; + const updateSkill = jest.fn(async () => ({ + status: 'updated' as const, + skill: { + _id: SKILL_ID, + name: 'runtime-skill', + body: updatedBody, + version: 2, + }, + warnings: [ + { + field: 'frontmatter.triger', + code: 'UNKNOWN_KEY', + severity: 'warning' as const, + message: '"triger" is not a recognized frontmatter key and is stored as-is', + }, + ], + })); + const handler = makeAuthoringHandler({ + getSkillByName: jest.fn(async () => ({ + _id: SKILL_ID, + name: 'runtime-skill', + body: oldBody, + fileCount: 0, + version: 1, + })), + updateSkill, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_edit_warning_skill', + name: 'edit_file', + args: { + path: 'skills/runtime-skill/SKILL.md', + old_text: 'description: Use before', + new_text: 'description: Use after\ntriger: manual', + }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).toContain('Warnings:'); + expect(result.content).toContain('frontmatter.triger [UNKNOWN_KEY]'); + expect(result.artifact).toMatchObject({ + warning_count: 1, + warnings: [ + expect.objectContaining({ + field: 'frontmatter.triger', + code: 'UNKNOWN_KEY', + severity: 'warning', + }), + ], + }); + }); + it('preserves block-scalar SKILL.md descriptions when editing skills', async () => { const oldBody = '---\nname: runtime-skill\ndescription: Use before\n---\n# Runtime skill\n'; const updateSkill = jest.fn(async () => ({ diff --git a/packages/api/src/agents/handlers.ts b/packages/api/src/agents/handlers.ts index bb28711928..1cca993bd4 100644 --- a/packages/api/src/agents/handlers.ts +++ b/packages/api/src/agents/handlers.ts @@ -1,7 +1,7 @@ import yaml from 'js-yaml'; import { Types } from 'mongoose'; -import { logger } from '@librechat/data-schemas'; import { GraphEvents, Constants } from '@librechat/agents'; +import { logger, normalizeSkillFrontmatterKeys } from '@librechat/data-schemas'; import type { LCTool, EventHandler, @@ -12,6 +12,7 @@ import type { ToolExecuteBatchRequest, } from '@librechat/agents'; import type { StructuredToolInterface } from '@librechat/agents/langchain/tools'; +import type { ValidationIssue } from '@librechat/data-schemas'; import type { CodeEnvRef } from 'librechat-data-provider'; import type { SkillFileRecord, PrimeSkillFilesResult } from './skillFiles'; import type { ServerRequest } from '~/types'; @@ -169,6 +170,7 @@ export interface ToolExecuteOptions { body: string; version: number; }; + warnings: ValidationIssue[]; }>; /** Updates a skill body and derived metadata from a tool-authored SKILL.md body. */ updateSkill?: (params: { @@ -184,6 +186,7 @@ export interface ToolExecuteOptions { | { status: 'updated'; skill: { _id: Types.ObjectId; name: string; body: string; version: number }; + warnings: ValidationIssue[]; } | { status: 'conflict'; current: { _id: Types.ObjectId; name: string; version: number } } | { status: 'not_found' } @@ -346,6 +349,10 @@ const MAX_AUTHORING_BYTES = 10 * 1024 * 1024; const MAX_TOOL_ERROR_MESSAGE_CHARS = 12_000; const MAX_TOOL_ERROR_STACK_CHARS = 4_000; const SKILL_MD = 'SKILL.md'; +const MAX_SKILL_AUTHORING_WARNINGS = 20; +const MAX_SKILL_WARNING_FIELD_CHARS = 120; +const MAX_SKILL_WARNING_CODE_CHARS = 64; +const MAX_SKILL_WARNING_MESSAGE_CHARS = 300; const IMAGE_MIMES = new Set(['image/png', 'image/jpeg', 'image/gif', 'image/webp']); @@ -611,6 +618,34 @@ function successResult( return result; } +function surfaceSkillAuthoringWarnings(warnings: ValidationIssue[] | undefined): { + contentSuffix: string; + warnings: Array; + warningCount: number; +} | null { + if (!warnings?.length) { + return null; + } + const surfaced = warnings.slice(0, MAX_SKILL_AUTHORING_WARNINGS).map((warning) => ({ + field: truncateMiddle(warning.field, MAX_SKILL_WARNING_FIELD_CHARS), + code: truncateMiddle(warning.code, MAX_SKILL_WARNING_CODE_CHARS), + message: truncateMiddle(warning.message, MAX_SKILL_WARNING_MESSAGE_CHARS), + severity: 'warning' as const, + })); + const omitted = warnings.length - surfaced.length; + const lines = surfaced.map( + (warning) => `- ${warning.field} [${warning.code}]: ${warning.message}`, + ); + if (omitted > 0) { + lines.push(`- ${omitted} additional warning(s) omitted.`); + } + return { + contentSuffix: `\n\nWarnings:\n${lines.join('\n')}`, + warnings: surfaced, + warningCount: warnings.length, + }; +} + function guessMimeType(filename: string): string { return MIME_MAP[lowercaseExtension(filename)] ?? 'application/octet-stream'; } @@ -813,7 +848,11 @@ function parseStructuredSkillFrontmatter( if (typeof parsed !== 'object' || Array.isArray(parsed)) { return { error: `${SKILL_MD} frontmatter must be a YAML mapping.` }; } - return { frontmatter: parsed as Record }; + const normalized = normalizeSkillFrontmatterKeys(parsed as Record); + if ('error' in normalized) { + return { error: `Invalid ${SKILL_MD} frontmatter: ${normalized.error}` }; + } + return { frontmatter: normalized.frontmatter }; } catch (error) { const message = error instanceof Error ? error.message : String(error); return { error: `Invalid ${SKILL_MD} frontmatter: ${message}` }; @@ -2410,13 +2449,20 @@ async function writeSkillMd({ throw error; } rememberAuthoredSkill([mergedConfigurable, sourceConfigurable], result.skill); + const surfacedWarnings = surfaceSkillAuthoringWarnings(result.warnings); return successResult( tc, - `Created ${SKILL_FILE_PREFIX}${skillName}/${SKILL_MD} (${content.length} chars).`, + `Created ${SKILL_FILE_PREFIX}${skillName}/${SKILL_MD} (${content.length} chars).${surfacedWarnings?.contentSuffix ?? ''}`, { path: `${SKILL_FILE_PREFIX}${skillName}/${SKILL_MD}`, bytes_written: Buffer.byteLength(content, 'utf8'), created: true, + ...(surfacedWarnings + ? { + warnings: surfacedWarnings.warnings, + warning_count: surfacedWarnings.warningCount, + } + : {}), }, ); } @@ -2455,11 +2501,19 @@ async function writeSkillMd({ content, ); const summary = `Updated ${SKILL_FILE_PREFIX}${skillName}/${SKILL_MD} (${content.length} chars).`; - return successResult(tc, diff ? `${summary}\n\n${diff}` : summary, { + const surfacedWarnings = surfaceSkillAuthoringWarnings(result.warnings); + const summaryWithWarnings = `${summary}${surfacedWarnings?.contentSuffix ?? ''}`; + return successResult(tc, diff ? `${summaryWithWarnings}\n\n${diff}` : summaryWithWarnings, { path: `${SKILL_FILE_PREFIX}${skillName}/${SKILL_MD}`, bytes_written: Buffer.byteLength(content, 'utf8'), created: false, ...(diff ? { diff } : {}), + ...(surfacedWarnings + ? { + warnings: surfacedWarnings.warnings, + warning_count: surfacedWarnings.warningCount, + } + : {}), }); } diff --git a/packages/api/src/skills/__tests__/deployment.test.ts b/packages/api/src/skills/__tests__/deployment.test.ts index ea180d240b..baf93dbcf9 100644 --- a/packages/api/src/skills/__tests__/deployment.test.ts +++ b/packages/api/src/skills/__tests__/deployment.test.ts @@ -159,15 +159,76 @@ describe('loadDeploymentSkillsFromDirectory', () => { }); }); - it('validates SKILL.md frontmatter at startup', async () => { + it('loads a skill with an unrecognized frontmatter key and warns about it', async () => { const root = await makeTempRoot(); await writeDeploymentSkill(root, { - name: 'bad-frontmatter', + name: 'unknown-key-frontmatter', frontmatter: [ '---', - 'name: bad-frontmatter', + 'name: unknown-key-frontmatter', `description: ${DESCRIPTION}`, 'unknown-key: nope', + 'references:', + ' - references/guide.txt', + '---', + '', + 'Body', + ].join('\n'), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + const registry = await loadDeploymentSkillsFromDirectory(path.join(root, 'skill'), { + projectRoot: root, + }); + + expect(registry.list().map((skill) => skill.name)).toEqual(['unknown-key-frontmatter']); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('frontmatter.unknown-key')); + warn.mockRestore(); + }); + + it('canonicalizes recognized frontmatter key variants before deriving runtime fields', async () => { + const root = await makeTempRoot(); + await writeDeploymentSkill(root, { + name: 'case-variant-frontmatter', + frontmatter: [ + '---', + 'name: case-variant-frontmatter', + `description: ${DESCRIPTION}`, + 'Allowed-Tools:', + ' - execute_code', + 'User-Invocable: false', + '---', + '', + 'Body', + ].join('\n'), + }); + + const registry = await loadDeploymentSkillsFromDirectory(path.join(root, 'skill'), { + projectRoot: root, + }); + + expect(registry.list()[0]).toMatchObject({ + allowedTools: ['execute_code'], + userInvocable: false, + frontmatter: { + 'allowed-tools': ['execute_code'], + 'user-invocable': false, + }, + }); + }); + + it('rejects case-colliding recognized frontmatter keys at startup', async () => { + const root = await makeTempRoot(); + await writeDeploymentSkill(root, { + name: 'case-collision-frontmatter', + frontmatter: [ + '---', + 'name: case-collision-frontmatter', + `description: ${DESCRIPTION}`, + 'allowed-tools:', + ' - read_file', + 'Allowed-Tools:', + ' - execute_code', '---', '', 'Body', @@ -176,7 +237,27 @@ describe('loadDeploymentSkillsFromDirectory', () => { await expect( loadDeploymentSkillsFromDirectory(path.join(root, 'skill'), { projectRoot: root }), - ).rejects.toThrow(/frontmatter\.unknown-key/); + ).rejects.toThrow(/both resolve to "allowed-tools"/); + }); + + it('rejects malformed SKILL.md frontmatter at startup', async () => { + const root = await makeTempRoot(); + await writeDeploymentSkill(root, { + name: 'bad-frontmatter', + frontmatter: [ + '---', + 'name: bad-frontmatter', + `description: ${DESCRIPTION}`, + 'user-invocable: maybe', + '---', + '', + 'Body', + ].join('\n'), + }); + + await expect( + loadDeploymentSkillsFromDirectory(path.join(root, 'skill'), { projectRoot: root }), + ).rejects.toThrow(/frontmatter\.user-invocable/); }); it('validates bundled file paths at startup', async () => { diff --git a/packages/api/src/skills/__tests__/import.test.ts b/packages/api/src/skills/__tests__/import.test.ts index 55416967d2..283a7ec31f 100644 --- a/packages/api/src/skills/__tests__/import.test.ts +++ b/packages/api/src/skills/__tests__/import.test.ts @@ -287,6 +287,21 @@ describe('parseFrontmatter', () => { ); }); + it('rejects case-colliding recognized frontmatter keys', () => { + const raw = `---\nname: duplicate-case\ndescription: Duplicate key variants.\nallowed-tools:\n - execute_code\nAllowed-Tools:\n - web_search\n---\n\nbody`; + + expect(parseFrontmatter(raw)).toEqual( + expect.objectContaining({ + name: '', + description: '', + invalidBooleans: [], + parseError: expect.stringContaining( + 'Recognized frontmatter keys "allowed-tools" and "Allowed-Tools" both resolve to "allowed-tools"', + ), + }), + ); + }); + it('ignores always-apply appearing outside the frontmatter block', () => { const raw = `---\nname: n\ndescription: d\n---\n\nalways-apply: true (but this is in the body)`; const result = parseFrontmatter(raw); diff --git a/packages/api/src/skills/deployment.ts b/packages/api/src/skills/deployment.ts index 195875502f..3152a53246 100644 --- a/packages/api/src/skills/deployment.ts +++ b/packages/api/src/skills/deployment.ts @@ -13,6 +13,7 @@ import { validateSkillFrontmatter, validateSkillDescription, deriveStructuredFrontmatterFields, + normalizeSkillFrontmatterKeys, } from '@librechat/data-schemas'; import type { ValidationIssue } from '@librechat/data-schemas'; import type { CodeEnvRef } from 'librechat-data-provider'; @@ -869,7 +870,11 @@ function parseStructuredFrontmatter( if (typeof parsed !== 'object' || Array.isArray(parsed)) { return { error: `${SKILL_MD} frontmatter must be a YAML mapping.` }; } - return { frontmatter: parsed as Record }; + const normalized = normalizeSkillFrontmatterKeys(parsed as Record); + if ('error' in normalized) { + return { error: `Invalid ${SKILL_MD} frontmatter: ${normalized.error}` }; + } + return { frontmatter: normalized.frontmatter }; } catch (error) { const message = error instanceof Error ? error.message : String(error); return { error: `Invalid ${SKILL_MD} frontmatter: ${message}` }; diff --git a/packages/api/src/skills/parse.ts b/packages/api/src/skills/parse.ts index 041e87ed5e..8540b8199c 100644 --- a/packages/api/src/skills/parse.ts +++ b/packages/api/src/skills/parse.ts @@ -1,4 +1,5 @@ import yaml from 'js-yaml'; +import { normalizeSkillFrontmatterKeys } from '@librechat/data-schemas'; export type ParsedSkillMarkdown = { name: string; @@ -64,14 +65,6 @@ function stripInlineComment(value: string): string { return value.trim(); } -function normalizeFrontmatterKeys(frontmatter: Record): Record { - return Object.entries(frontmatter).reduce>((acc, [key, value]) => { - const normalizedKey = key.toLowerCase(); - acc[normalizedKey === 'alwaysapply' ? 'alwaysApply' : normalizedKey] = value; - return acc; - }, {}); -} - function parseBoolean(value: unknown, rawValue?: string): boolean | undefined { const raw = rawValue === undefined ? undefined : stripInlineComment(rawValue).toLowerCase(); if (typeof value === 'boolean') { @@ -120,7 +113,19 @@ export function parseSkillMarkdown(raw: string): ParsedSkillMarkdown { parseError: error instanceof Error ? error.message : 'Invalid YAML frontmatter', }; } - const frontmatter = isPlainObject(parsed) ? normalizeFrontmatterKeys(parsed) : {}; + let frontmatter: Record = {}; + if (isPlainObject(parsed)) { + const normalized = normalizeSkillFrontmatterKeys(parsed); + if ('error' in normalized) { + return { + name: '', + description: '', + invalidBooleans: [], + parseError: normalized.error, + }; + } + frontmatter = normalized.frontmatter; + } const nameValue = getCaseInsensitive(frontmatter, 'name'); const descriptionValue = getCaseInsensitive(frontmatter, 'description'); const whenToUseValue = getCaseInsensitive(frontmatter, 'when-to-use'); diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index d0c8e946a6..76e4c16787 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -1,6 +1,6 @@ import crypto from 'crypto'; import { Types } from 'mongoose'; -import { getTenantId } from '@librechat/data-schemas'; +import { logger, getTenantId } from '@librechat/data-schemas'; import type { ISkill, ISkillFile, @@ -10,6 +10,7 @@ import type { SkillSyncStatusInput, UpdateSkillInput, UpdateSkillResult, + UpsertSkillFileInput, } from '@librechat/data-schemas'; import type { GitHubSkillSyncDeps } from './github'; import { DEFAULT_SKILL_IMPORT_LIMITS } from '../limits'; @@ -95,6 +96,72 @@ function githubFetch( }) as unknown as typeof fetch; } +/** Serves one `skills//SKILL.md` per entry, in the order given. */ +function multiSkillFetch( + skills: Array<{ dir: string; markdown: string }>, + { + rateLimitedDirs = [], + requestFailedDirs = [], + rejectedDirs = [], + }: { + rateLimitedDirs?: string[]; + requestFailedDirs?: string[]; + rejectedDirs?: string[]; + } = {}, +): typeof fetch { + return 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: skills.map(({ dir, markdown }) => ({ + path: `${dir}/SKILL.md`, + mode: '100644', + type: 'blob', + sha: `${dir}-skill-sha`, + size: Buffer.byteLength(markdown), + url: `https://api.github.test/blob/${dir}`, + })), + }); + } + const requested = skills.find(({ dir }) => url.includes(`/git/blobs/${dir}-skill-sha`)); + if (requested && rateLimitedDirs.includes(requested.dir)) { + return response({ message: 'API rate limit exceeded' }, 403, { + 'x-ratelimit-remaining': '0', + }); + } + if (requested && requestFailedDirs.includes(requested.dir)) { + return response({ message: 'upstream unavailable' }, 503); + } + if (requested && rejectedDirs.includes(requested.dir)) { + throw new TypeError('fetch failed'); + } + if (requested) { + return response(blob(requested.markdown)); + } + return response({ message: 'not found' }, 404); + }) as unknown as typeof fetch; +} + function makeSkill(input: CreateSkillInput): ISkill & { _id: Types.ObjectId } { return { _id: new Types.ObjectId(), @@ -192,7 +259,8 @@ function createDeps( paths: input.paths, startedAt: input.startedAt, finishedAt: input.finishedAt, - lastSuccessAt: input.status === 'succeeded' ? input.finishedAt : undefined, + lastSuccessAt: + input.status === 'succeeded' || input.status === 'partial' ? input.finishedAt : undefined, lastFailureAt: input.status === 'failed' ? input.finishedAt : undefined, errorCode: input.errorCode, errorMessage: input.errorMessage, @@ -200,6 +268,8 @@ function createDeps( syncedFileCount: input.syncedFileCount ?? 0, deletedSkillCount: input.deletedSkillCount ?? 0, deletedFileCount: input.deletedFileCount ?? 0, + skippedSkillCount: input.skippedSkillCount ?? 0, + skippedSkills: input.skippedSkills, }; statuses.push(status); return status; @@ -308,6 +378,49 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('preserves unknown frontmatter key casing while canonicalizing recognized keys', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\nAllowed-Tools:\n - execute_code\ncustomConfig: camel\ncustomconfig: lower\n---\nBody', + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + frontmatter: expect.objectContaining({ + 'allowed-tools': ['execute_code'], + customConfig: 'camel', + customconfig: 'lower', + }), + }), + ); + }); + + it('rejects case-colliding recognized frontmatter keys instead of choosing by order', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\nallowed-tools:\n - execute_code\nAllowed-Tools:\n - web_search\n---\nBody', + ), + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.createSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: expect.stringContaining( + 'Recognized frontmatter keys "allowed-tools" and "Allowed-Tools" both resolve to "allowed-tools"', + ), + }), + ); + }); + it('fails duplicate discovered skill names before publishing partial mirrors', async () => { const duplicateFetch = jest.fn(async (input: RequestInfo | URL) => { const url = input.toString(); @@ -450,6 +563,594 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('publishes the healthy skills of a source and records the ones it had to skip', async () => { + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { dir: 'broken', markdown: '---\nname: [\n---\nBody' }, + { + dir: 'analysis', + markdown: '---\nname: analysis\ndescription: Analyze things\n---\nBody', + }, + ]), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect((deps.createSkill as jest.Mock).mock.calls.map(([input]) => input.name)).toEqual([ + 'research', + 'analysis', + ]); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 2, + skippedSkillCount: 1, + skippedSkills: [ + expect.objectContaining({ + path: 'skills/broken', + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: expect.stringContaining('skills/broken/SKILL.md'), + }), + ], + }), + ); + }); + + it('bounds a skipped skill path so the partial status remains persistable', async () => { + const longDirectory = 'a'.repeat(600); + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { dir: longDirectory, markdown: '---\nname: [\n---\nBody' }, + ]), + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 1, + skippedSkills: [ + expect.objectContaining({ + path: expect.stringMatching(/^skills\/a+…$/), + }), + ], + }), + ); + const statusCalls = (deps.upsertStatus as jest.Mock).mock.calls; + const status = statusCalls[statusCalls.length - 1]?.[0] as SkillSyncStatusInput; + expect(status.skippedSkills?.[0].path).toHaveLength(500); + }); + + it('escapes control characters in skipped skill diagnostics', async () => { + const maliciousDirectory = 'broken\n\x1b[31mforged\u2028line\u2029paragraph'; + const skillMarkdown = '---\nname: broken\ndescription: Broken skill\n---\nBody'; + 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: 'healthy/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'healthy-skill-sha', + size: Buffer.byteLength(skillMarkdown), + url: 'https://api.github.test/blob/healthy-skill', + }, + { + path: `${maliciousDirectory}/SKILL.md`, + mode: '100644', + type: 'blob', + sha: 'malicious-skill-sha', + size: Buffer.byteLength(skillMarkdown), + url: 'https://api.github.test/blob/malicious-skill', + }, + ], + }); + } + if (url.includes('/git/blobs/malicious-skill-sha')) { + return response(blob(skillMarkdown)); + } + if (url.includes('/git/blobs/healthy-skill-sha')) { + return response(blob('---\nname: healthy\ndescription: Healthy skill\n---\nHealthy body')); + } + return response({ message: 'not found' }, 404); + }) as unknown as typeof fetch; + const deps = createDeps({ + fetchFn, + createSkill: jest.fn(async (input: CreateSkillInput): Promise => { + if (input.name === 'broken') { + throw new Error('validation failed\n\x1b[2Jforged\u2028line\u2029paragraph'); + } + return { skill: makeSkill(input), warnings: [] }; + }), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + try { + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + const warning = warn.mock.calls + .map(([message]) => String(message)) + .find((message) => message.includes(' skipped "')); + const statusCalls = (deps.upsertStatus as jest.Mock).mock.calls; + const status = statusCalls[statusCalls.length - 1]?.[0] as SkillSyncStatusInput; + + expect(result.status).toBe('completed'); + expect(warning).toContain('skills/broken\\n\\u001b[31mforged\\u2028line\\u2029paragraph'); + expect(warning).toContain('validation failed\\n\\u001b[2Jforged\\u2028line\\u2029paragraph'); + expect( + [...(warning ?? '')].every((character) => { + const codePoint = character.charCodeAt(0); + return !( + (codePoint >= 0 && codePoint <= 0x1f) || + (codePoint >= 0x7f && codePoint <= 0x9f) || + codePoint === 0x2028 || + codePoint === 0x2029 + ); + }), + ).toBe(true); + expect(status.skippedSkills?.[0]).toEqual( + expect.objectContaining({ + path: 'skills/broken\\n\\u001b[31mforged\\u2028line\\u2029paragraph', + errorMessage: 'validation failed\\n\\u001b[2Jforged\\u2028line\\u2029paragraph', + }), + ); + } finally { + warn.mockRestore(); + } + }); + + it('bounds and caps per-skill warning logs for a pathological source', async () => { + const invalidSkills = Array.from({ length: 25 }, (_, index) => ({ + dir: `${index}-${'x'.repeat(600)}`, + markdown: '---\nname: [\n---\nBody', + })); + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + ...invalidSkills, + ]), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + try { + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + const warningText = warn.mock.calls.map(([message]) => String(message)); + const perSkillWarnings = warningText.filter((message) => message.includes(' skipped "')); + + expect(result.status).toBe('completed'); + expect(perSkillWarnings).toHaveLength(20); + expect(perSkillWarnings.every((message) => message.length <= 1100)).toBe(true); + expect(warningText).toContain( + '[GitHubSkillSync] Source "librechat-skills" suppressed 5 additional skipped skill warning(s)', + ); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 25, + skippedSkills: expect.any(Array), + }), + ); + const statusCalls = (deps.upsertStatus as jest.Mock).mock.calls; + const status = statusCalls[statusCalls.length - 1]?.[0] as SkillSyncStatusInput; + expect(status.skippedSkills).toHaveLength(20); + } finally { + warn.mockRestore(); + } + }); + + it('bounds a skipped skill name so the partial status remains persistable', async () => { + const longName = 'b'.repeat(600); + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { + dir: 'oversized-name', + markdown: `---\nname: ${longName}\ndescription: Invalid name\n---\nBody`, + }, + ]), + createSkill: jest.fn(async (input: CreateSkillInput): Promise => { + if (input.name === longName) { + throw new Error('Skill validation failed'); + } + return { skill: makeSkill(input), warnings: [] }; + }), + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('completed'); + const statusCalls = (deps.upsertStatus as jest.Mock).mock.calls; + const status = statusCalls[statusCalls.length - 1]?.[0] as SkillSyncStatusInput; + expect(status).toEqual( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 1, + }), + ); + expect(status.skippedSkills?.[0].name).toMatch(/^b+…$/); + expect(status.skippedSkills?.[0].name).toHaveLength(128); + }); + + it('preserves bounded validation details for a skipped skill', async () => { + const validationError = new Error('Skill validation failed') as Error & { + code: string; + issues: Array<{ field: string; code: string; message: string }>; + }; + validationError.code = 'SKILL_VALIDATION_FAILED'; + validationError.issues = [ + { + field: 'frontmatter.alwaysApply', + code: 'INVALID_TYPE', + message: '"always-apply" must be a boolean', + }, + { + field: 'body', + code: 'INVALID_BODY', + message: `Bearer github_pat_secret ${'x'.repeat(700)}`, + }, + ]; + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { + dir: 'broken', + markdown: '---\nname: broken\ndescription: Broken\n---\nBody', + }, + ]), + createSkill: jest.fn(async (input: CreateSkillInput): Promise => { + if (input.name === 'broken') { + throw validationError; + } + return { skill: makeSkill(input), warnings: [] }; + }), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + try { + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + const statusCalls = (deps.upsertStatus as jest.Mock).mock.calls; + const status = statusCalls[statusCalls.length - 1]?.[0] as SkillSyncStatusInput; + const skipped = status.skippedSkills?.[0]; + const warningText = warn.mock.calls.map(([message]) => String(message)).join('\n'); + + expect(result.status).toBe('completed'); + expect(status).toEqual( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 1, + }), + ); + expect(skipped).toEqual( + expect.objectContaining({ + path: 'skills/broken', + name: 'broken', + errorCode: 'SKILL_VALIDATION_FAILED', + }), + ); + expect(skipped?.errorMessage).toContain('frontmatter.alwaysApply [INVALID_TYPE]'); + expect(skipped?.errorMessage).toContain('Bearer [redacted]'); + expect(skipped?.errorMessage).not.toContain('github_pat_secret'); + expect(skipped?.errorMessage?.length).toBeLessThanOrEqual(500); + expect(skipped?.errorMessage).toMatch(/…$/); + expect(warningText).toContain('frontmatter.alwaysApply [INVALID_TYPE]'); + expect(warningText).toContain('Bearer [redacted]'); + expect(warningText).not.toContain('github_pat_secret'); + } finally { + warn.mockRestore(); + } + }); + + it('logs the validation warnings of a synced skill instead of swallowing them', async () => { + /* An unrecognized frontmatter key no longer fails the skill, so the log is + the only place a maintainer learns the upstream SKILL.md carries one: + a background run has no user-facing surface to report it on. */ + const deps = createDeps({ + createSkill: jest.fn(async (input: CreateSkillInput): Promise => { + return { + skill: makeSkill(input), + warnings: [ + { + field: 'frontmatter.references', + code: 'UNKNOWN_KEY', + severity: 'warning', + message: '"references" is not a recognized frontmatter key', + }, + ], + }; + }), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + try { + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('completed'); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining( + 'frontmatter.references [UNKNOWN_KEY]: "references" is not a recognized', + ), + ); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('research')); + } finally { + warn.mockRestore(); + } + }); + + it('does not log new-skill warnings when publication rolls back', async () => { + const deps = createDeps({ + createSkill: jest.fn(async (input: CreateSkillInput): Promise => { + return { + skill: makeSkill(input), + warnings: [ + { + field: 'frontmatter.references', + code: 'UNKNOWN_KEY', + severity: 'warning', + message: '"references" is not a recognized frontmatter key', + }, + ], + }; + }), + grantPermission: jest.fn(async () => { + throw new Error('permission unavailable'); + }), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + try { + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + const warningText = warn.mock.calls.map(([message]) => String(message)); + + expect(result.status).toBe('failed'); + expect(deps.deleteSkill).toHaveBeenCalledTimes(1); + expect(warningText.some((message) => message.includes(' synced with warnings:'))).toBe(false); + } finally { + warn.mockRestore(); + } + }); + + it('bounds and caps validation warning logs across successfully synced skills', async () => { + const skills = Array.from({ length: 25 }, (_, index) => ({ + dir: `skill-${index}`, + markdown: `---\nname: skill-${index}\ndescription: Valid skill ${index}\n---\nBody`, + })); + const deps = createDeps({ + fetchFn: multiSkillFetch(skills), + createSkill: jest.fn( + async (input: CreateSkillInput): Promise => ({ + skill: makeSkill(input), + warnings: [ + { + field: `frontmatter.${'f'.repeat(300)}`, + code: 'UNKNOWN_KEY', + severity: 'warning', + message: `Unknown key ${'m'.repeat(1000)}`, + }, + ], + }), + ), + }); + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => logger); + + try { + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + const warningText = warn.mock.calls.map(([message]) => String(message)); + const validationWarnings = warningText.filter((message) => + message.includes(' synced with warnings:'), + ); + + expect(result.status).toBe('completed'); + expect(validationWarnings).toHaveLength(20); + expect(validationWarnings.every((message) => message.length <= 700)).toBe(true); + expect(warningText).toContain( + '[GitHubSkillSync] Source "librechat-skills" suppressed 5 additional synced skill validation warning(s)', + ); + } finally { + warn.mockRestore(); + } + }); + + it('keeps the previously synced mirror of a skipped skill instead of reconciling it away', async () => { + const author = makeSourceAuthorId(); + const brokenMirror = makeSkill({ + name: 'broken', + description: 'Previously valid', + author, + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/broken', + }, + }); + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { dir: 'broken', markdown: '---\nname: [\n---\nBody' }, + ]), + listSkillsBySource: jest.fn(async () => [brokenMirror]), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.deleteSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'partial', + deletedSkillCount: 0, + skippedSkillCount: 1, + }), + ); + }); + + it('skips every member of a duplicate name group and still publishes the unique skills', async () => { + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { dir: 'first', markdown: '---\nname: duplicate\ndescription: First\n---\nBody' }, + { dir: 'unique', markdown: '---\nname: unique\ndescription: Unique skill\n---\nBody' }, + { dir: 'second', markdown: '---\nname: duplicate\ndescription: Second\n---\nBody' }, + ]), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect((deps.createSkill as jest.Mock).mock.calls.map(([input]) => input.name)).toEqual([ + 'unique', + ]); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 2, + skippedSkills: [ + expect.objectContaining({ path: 'skills/first', errorCode: 'DUPLICATE_SKILL_NAME' }), + expect.objectContaining({ path: 'skills/second', errorCode: 'DUPLICATE_SKILL_NAME' }), + ], + }), + ); + }); + + it('fails the whole source when GitHub starts rate limiting part way through', async () => { + const deps = createDeps({ + fetchFn: multiSkillFetch( + [ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { dir: 'analysis', markdown: '---\nname: analysis\ndescription: Analyze\n---\nBody' }, + ], + { rateLimitedDirs: ['analysis'] }, + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + /* A refusal that will hit every remaining request is not the fault of the + skill that ran into it first, so it must not be filed as one skipped + skill on an otherwise healthy run. */ + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'GITHUB_RATE_LIMITED', + skippedSkillCount: 0, + }), + ); + }); + + it.each([ + ['returns a server error', { requestFailedDirs: ['analysis'] }], + ['rejects the request', { rejectedDirs: ['analysis'] }], + ])('fails the whole source when GitHub %s', async (_description, fetchOptions) => { + const deps = createDeps({ + fetchFn: multiSkillFetch( + [ + { + dir: 'research', + markdown: '---\nname: research\ndescription: Research things\n---\nBody', + }, + { dir: 'analysis', markdown: '---\nname: analysis\ndescription: Analyze\n---\nBody' }, + ], + fetchOptions, + ), + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'GITHUB_REQUEST_FAILED', + skippedSkillCount: 0, + }), + ); + }); + + it('preserves earlier skipped skill details when a later request is rate limited', async () => { + const deps = createDeps({ + fetchFn: multiSkillFetch( + [ + { dir: 'broken', markdown: '---\nname: [\n---\nBody' }, + { + dir: 'analysis', + markdown: '---\nname: analysis\ndescription: Analyze\n---\nBody', + }, + ], + { rateLimitedDirs: ['analysis'] }, + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'GITHUB_RATE_LIMITED', + skippedSkillCount: 1, + skippedSkills: [ + expect.objectContaining({ + path: 'skills/broken', + errorCode: 'SKILL_PARSE_FAILED', + }), + ], + }), + ); + }); + it('discovers nested skill roots within the configured discovery depth', async () => { const skillMarkdown = '---\nname: tdd\ndescription: Test-driven development\n---\nBody'; const fetchFn = jest.fn(async (input: RequestInfo | URL) => { @@ -947,7 +1648,7 @@ describe('createGitHubSkillSyncRunner', () => { ); }); - it('does not delete stale name-conflicting mirrors before another skill file sync fails', async () => { + it('keeps a failed skill mirror while still reconciling a mirror whose upstream root is gone', async () => { const renamedMarkdown = '---\nname: renamed\ndescription: Renamed skill\n---\nBody'; const brokenMarkdown = '---\nname: broken\ndescription: Broken skill\n---\nBody'; const fetchFn = jest.fn(async (input: RequestInfo | URL) => { @@ -1072,17 +1773,22 @@ describe('createGitHubSkillSyncRunner', () => { const result = await runner.runOnce(); expect(result.status).toBe('failed'); - expect(deleteSkill).not.toHaveBeenCalledWith(staleId.toString()); + /* The failed skill is still present upstream, so its mirror survives for a + later run to repair. The stale mirror is a different question: its + upstream root is gone, so reconciling it away is correct regardless of + which skills failed. */ + expect(deleteSkill).not.toHaveBeenCalledWith(existingId.toString()); expect(deps.updateSkill).not.toHaveBeenCalled(); expect(deps.upsertStatus).toHaveBeenLastCalledWith( expect.objectContaining({ status: 'failed', errorMessage: 'storage unavailable', + skippedSkillCount: 2, }), ); }); - it('restores a stale name-conflicting mirror when the rename update fails after deletion', async () => { + it('fails the source when recreating a stale mirror cannot preserve its dependent state', async () => { const staleId = new Types.ObjectId(); const existingId = new Types.ObjectId(); const author = makeSourceAuthorId(); @@ -1105,15 +1811,17 @@ describe('createGitHubSkillSyncRunner', () => { }; const staleSkill = makeExisting('librechat-skills:skills/removed', staleId, 'renamed'); const syncedSkill = makeExisting('librechat-skills:skills/research', existingId, 'research'); - const deletedIds = new Set(); + const persistedSkills = new Map( + [staleSkill, syncedSkill].map((skill) => [skill._id.toString(), skill]), + ); let restoredSkill: (ISkill & { _id: Types.ObjectId }) | undefined; const createSkill = jest.fn(async (input: CreateSkillInput): Promise => { restoredSkill = makeSkill(input); + persistedSkills.set(restoredSkill._id.toString(), restoredSkill); return { skill: restoredSkill, warnings: [] }; }); const deleteSkill = jest.fn(async (id: string) => { - deletedIds.add(id); - return { deleted: true }; + return { deleted: persistedSkills.delete(id) }; }); const deps = createDeps({ fetchFn: githubFetch('---\nname: renamed\ndescription: Renamed skill\n---\nBody'), @@ -1123,9 +1831,7 @@ describe('createGitHubSkillSyncRunner', () => { getSkillById: jest.fn(async (id) => id.toString() === existingId.toString() ? syncedSkill : null, ), - listSkillsBySource: jest.fn(async () => - [staleSkill, syncedSkill].filter((skill) => !deletedIds.has(skill._id.toString())), - ), + listSkillsBySource: jest.fn(async () => [...persistedSkills.values()]), createSkill, deleteSkill, updateSkill: jest.fn(async () => ({ status: 'conflict' as const, current: syncedSkill })), @@ -1135,6 +1841,7 @@ describe('createGitHubSkillSyncRunner', () => { expect(result.status).toBe('failed'); expect(deleteSkill).toHaveBeenCalledWith(staleId.toString()); + expect(deleteSkill).toHaveBeenCalledTimes(1); expect(createSkill).toHaveBeenCalledWith( expect.objectContaining({ name: 'renamed', @@ -1146,6 +1853,71 @@ describe('createGitHubSkillSyncRunner', () => { expect(deps.grantPermission).toHaveBeenCalledWith( expect.objectContaining({ resourceId: restoredSkill?._id }), ); + expect(persistedSkills.has(restoredSkill?._id.toString() ?? '')).toBe(true); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SYNC_ROLLBACK_FAILED', + errorMessage: 'Rollback failed after: Skill "research" changed during sync', + deletedSkillCount: 0, + deletedFileCount: 0, + }), + ); + }); + + it('fails the source when stale mirror deletion can leave partial persisted state', async () => { + const staleId = new Types.ObjectId(); + const existingId = new Types.ObjectId(); + const author = makeSourceAuthorId(); + const makeExisting = ( + upstreamId: string, + _id: Types.ObjectId, + name: string, + ): ISkill & { _id: Types.ObjectId } => { + const skill = makeSkill({ + name, + description: `${name} skill`, + author, + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { provider: 'github', sourceId: 'librechat-skills', upstreamId }, + }); + skill._id = _id; + return skill; + }; + const staleSkill = makeExisting('librechat-skills:skills/removed', staleId, 'renamed'); + const syncedSkill = makeExisting('librechat-skills:skills/research', existingId, 'research'); + const deleteSkill = jest.fn(async (id: string) => { + if (id === staleId.toString()) { + throw new Error('skill file deletion unavailable'); + } + return { deleted: true }; + }); + const deps = createDeps({ + fetchFn: githubFetch('---\nname: renamed\ndescription: Renamed skill\n---\nBody'), + findSkillBySourceIdentity: jest.fn(async ({ upstreamId }) => + upstreamId === 'librechat-skills:skills/research' ? syncedSkill : null, + ), + getSkillById: jest.fn(async (id) => + id.toString() === existingId.toString() ? syncedSkill : null, + ), + listSkillsBySource: jest.fn(async () => [staleSkill, syncedSkill]), + deleteSkill, + updateSkill: jest.fn(), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deleteSkill).toHaveBeenCalledWith(staleId.toString()); + expect(deps.updateSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SYNC_ROLLBACK_FAILED', + errorMessage: 'Stale mirror deletion failed: skill file deletion unavailable', + }), + ); }); it("does not mirror-delete another tenant's skills from an ambient source run", async () => { @@ -1333,21 +2105,91 @@ describe('createGitHubSkillSyncRunner', () => { }); it('marks a source failed and skips mirror deletion when SKILL.md frontmatter is malformed', async () => { + const movedMirror = makeSkill({ + name: 'research', + description: 'Last known good description', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/old-research', + }, + }) as ISkill & { _id: Types.ObjectId }; const deps = createDeps({ fetchFn: githubFetch('---\nname: [\n---\nBody'), + listSkillsBySource: jest.fn(async () => [movedMirror]), }); const runner = createGitHubSkillSyncRunner(deps); const result = await runner.runOnce(); expect(result.status).toBe('failed'); expect(deps.createSkill).not.toHaveBeenCalled(); - expect(deps.listSkillsBySource).not.toHaveBeenCalled(); expect(deps.deleteSkill).not.toHaveBeenCalled(); expect(deps.upsertStatus).toHaveBeenLastCalledWith( expect.objectContaining({ status: 'failed', errorCode: 'SKILL_PARSE_FAILED', errorMessage: expect.stringContaining('skills/research/SKILL.md'), + skippedSkillCount: 1, + }), + ); + }); + + it('does not create a conflicting moved skill after another skill fails preparation', async () => { + const lastKnownGood = makeSkill({ + name: 'research', + description: 'Last known good description', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/old-research', + }, + }) as ISkill & { _id: Types.ObjectId }; + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { dir: 'broken', markdown: '---\nname: [\n---\nBody' }, + { + dir: 'healthy', + markdown: '---\nname: research\ndescription: Healthy replacement candidate\n---\nBody', + }, + { + dir: 'unique', + markdown: '---\nname: analysis\ndescription: Independent healthy skill\n---\nBody', + }, + ]), + listSkillsBySource: jest.fn(async () => [lastKnownGood]), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.updateSkill).not.toHaveBeenCalled(); + expect(deps.deleteSkill).not.toHaveBeenCalled(); + expect(deps.createSkill).toHaveBeenCalledTimes(1); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'analysis', + sourceMetadata: expect.objectContaining({ + upstreamId: 'librechat-skills:skills/unique', + }), + }), + ); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 2, + skippedSkills: expect.arrayContaining([ + expect.objectContaining({ + path: 'skills/healthy', + errorCode: 'SKILL_MOVE_AMBIGUOUS', + }), + ]), }), ); }); @@ -1631,6 +2473,258 @@ describe('createGitHubSkillSyncRunner', () => { expect(deps.deleteSkill).not.toHaveBeenCalled(); }); + it('fails the source when a skill rollback leaves a half-written mirror', async () => { + /* A clean rollback is just a skipped skill. A failed one leaves the mirror + inconsistent, which must not be reported as a partial success next to + the skills that did publish. */ + const deps = createDeps({ + saveBuffer: jest.fn(async () => { + throw new Error('storage unavailable'); + }), + deleteSkill: jest.fn(async () => { + throw new Error('rollback unavailable'); + }), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SYNC_ROLLBACK_FAILED', + errorMessage: expect.stringContaining('storage unavailable'), + }), + ); + }); + + it('fails the source when rollback cannot remove a stored skill file', async () => { + const storedFiles: Array = []; + const deleteFile = jest.fn(async () => { + throw new Error('storage cleanup unavailable'); + }); + const deleteSkill = jest.fn(async () => ({ deleted: true })); + const deps = createDeps({ + listSkillFiles: jest.fn(async () => storedFiles), + upsertSkillFile: jest.fn(async (input: UpsertSkillFileInput) => { + const file = { ...input, _id: new Types.ObjectId() } as ISkillFile & { + _id: Types.ObjectId; + }; + storedFiles.push(file); + return file; + }), + grantPermission: jest.fn(async () => { + throw new Error('permission unavailable'); + }), + deleteFile, + deleteSkill, + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deleteFile).toHaveBeenCalledTimes(1); + expect(deleteSkill).toHaveBeenCalledTimes(1); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SYNC_ROLLBACK_FAILED', + errorMessage: 'Rollback failed after: permission unavailable', + }), + ); + }); + + it('fails the source when an unpersisted upload cannot be cleaned up', async () => { + const deleteFile = jest.fn(async () => { + throw new Error('orphan cleanup unavailable'); + }); + const deps = createDeps({ + upsertSkillFile: jest.fn(async () => { + throw new Error('database unavailable'); + }), + deleteFile, + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('failed'); + expect(deleteFile).toHaveBeenCalledTimes(1); + expect(deps.deleteSkill).toHaveBeenCalledTimes(1); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SYNC_ROLLBACK_FAILED', + errorMessage: 'Rollback failed after: database unavailable', + }), + ); + }); + + it('keeps a moved skill mirror when the name it moves into turns out to be duplicated', async () => { + /* Both discovered paths claim the same name, so neither publishes. The + mirror the move would have reused is still live and must survive. */ + const existing = makeSkill({ + name: 'duplicate', + description: 'Old description', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + frontmatter: {}, + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/old-duplicate', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/old-duplicate', + }, + }) as ISkill & { _id: Types.ObjectId }; + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { dir: 'first', markdown: '---\nname: duplicate\ndescription: First\n---\nBody' }, + { dir: 'second', markdown: '---\nname: duplicate\ndescription: Second\n---\nBody' }, + ]), + findSkillBySourceIdentity: jest.fn(async () => null), + listSkillsBySource: jest.fn(async () => [existing]), + getSkillById: jest.fn(async () => existing), + updateSkill: jest.fn(), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.createSkill).not.toHaveBeenCalled(); + expect(deps.deleteSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ deletedSkillCount: 0, skippedSkillCount: 2 }), + ); + }); + + it('keeps a moved and renamed mirror when duplicate replacements are skipped', async () => { + const existing = makeSkill({ + name: 'old-research', + description: 'Last known good description', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + frontmatter: {}, + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/old-research', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/old-research', + }, + }) as ISkill & { _id: Types.ObjectId }; + const deps = createDeps({ + fetchFn: multiSkillFetch([ + { dir: 'first', markdown: '---\nname: duplicate\ndescription: First\n---\nBody' }, + { dir: 'second', markdown: '---\nname: duplicate\ndescription: Second\n---\nBody' }, + ]), + findSkillBySourceIdentity: jest.fn(async () => null), + listSkillsBySource: jest.fn(async () => [existing]), + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.createSkill).not.toHaveBeenCalled(); + expect(deps.deleteSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ deletedSkillCount: 0, skippedSkillCount: 2 }), + ); + }); + + it('keeps a moved skill mirror when its move fails part way through', async () => { + /* The mirror still carries the old upstream id until the update lands, so + a failed move must not leave the reconcile pass reading it as stale. */ + const existing = makeSkill({ + name: 'research', + description: 'Old description', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + frontmatter: {}, + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/old-research', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/old-research', + }, + }) as ISkill & { _id: Types.ObjectId }; + const deps = createDeps({ + findSkillBySourceIdentity: jest.fn(async () => null), + listSkillsBySource: jest.fn(async () => [existing]), + getSkillById: jest.fn(async () => existing), + listSkillFiles: jest.fn(async () => []), + getSkillFileByPath: jest.fn(async () => null), + saveBuffer: jest.fn(async () => { + throw new Error('storage unavailable'); + }), + updateSkill: jest.fn(), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.deleteSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ deletedSkillCount: 0, skippedSkillCount: 1 }), + ); + }); + + it('keeps a moved and renamed mirror when its replacement fails after preparation', async () => { + const existing = makeSkill({ + name: 'old-research', + description: 'Last known good description', + author: makeSourceAuthorId(), + authorName: 'GitHub Sync', + frontmatter: {}, + source: 'github', + sourceMetadata: { + provider: 'github', + sourceId: 'librechat-skills', + upstreamId: 'librechat-skills:skills/old-research', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + skillPath: 'skills/old-research', + }, + }) as ISkill & { _id: Types.ObjectId }; + let createdSkill: (ISkill & { _id: Types.ObjectId }) | undefined; + const deleteSkill = jest.fn(async () => ({ deleted: true })); + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: renamed-research\ndescription: Renamed research skill\n---\nBody', + ), + findSkillBySourceIdentity: jest.fn(async () => null), + listSkillsBySource: jest.fn(async () => [existing]), + createSkill: jest.fn(async (input: CreateSkillInput): Promise => { + createdSkill = makeSkill(input); + return { skill: createdSkill, warnings: [] }; + }), + saveBuffer: jest.fn(async () => { + throw new Error('storage unavailable'); + }), + deleteSkill, + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(createdSkill).toBeDefined(); + expect(deleteSkill).toHaveBeenCalledWith(createdSkill!._id.toString()); + expect(deleteSkill).not.toHaveBeenCalledWith(existing._id.toString()); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ deletedSkillCount: 0, skippedSkillCount: 1 }), + ); + }); + it('refreshes an existing skill version after file sync before updating metadata', async () => { const existing = makeSkill({ name: 'research', @@ -1896,6 +2990,62 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('fails the source when existing-skill rollback cannot remove replacement storage', 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 deleteFile = jest.fn(async () => { + throw new Error('replacement cleanup unavailable'); + }); + const deps = createDeps({ + findSkillBySourceIdentity: jest.fn(async () => existing), + getSkillById: jest.fn(async () => ({ ...existing, version: existing.version + 1 })), + getSkillFileByPath: jest.fn(async () => oldFile), + listSkillFiles: jest.fn(async () => [oldFile]), + upsertSkillFile: jest.fn(async (row) => ({ + ...oldFile, + ...row, + _id: oldFile._id, + skillId: row.skillId as Types.ObjectId, + })), + saveBuffer: jest.fn(async () => ({ + filepath: '/uploads/new-file-id__run.sh', + source: 'local', + })), + deleteFile, + updateSkill: jest.fn(async () => ({ status: 'conflict' as const, current: existing })), + }); + + const result = await createGitHubSkillSyncRunner(deps).runOnce(); + + expect(result.status).toBe('failed'); + expect(deleteFile).toHaveBeenCalledWith( + expect.objectContaining({ filepath: '/uploads/new-file-id__run.sh' }), + ); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SYNC_ROLLBACK_FAILED', + errorMessage: 'Rollback failed after: Skill "research" changed during sync', + }), + ); + }); + it('preserves credential presence when a manual run is skipped by an active lock', async () => { const deps = createDeps({ tryAcquireLock: jest.fn(async () => false), @@ -2142,7 +3292,6 @@ describe('createGitHubSkillSyncRunner', () => { expect(fetchedUrls.some((url) => url.includes('/git/blobs/oversized-file-sha'))).toBe(false); expect(deps.createSkill).not.toHaveBeenCalled(); expect(deps.saveBuffer).not.toHaveBeenCalled(); - expect(deps.listSkillsBySource).not.toHaveBeenCalled(); expect(deps.upsertStatus).toHaveBeenLastCalledWith( expect.objectContaining({ status: 'failed', diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 00cfefa907..4009b555ea 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -11,6 +11,8 @@ import { import type { ISkill, ISkillFile, + ValidationIssue, + ISkillSyncSkippedSkill, CreateSkillInput, UpdateSkillInput, CreateSkillResult, @@ -37,6 +39,17 @@ function getSystemAuthorId(): Types.ObjectId { } const PROVIDER: SkillSyncProvider = 'github'; const LOCK_LEASE_MS = 30 * 60 * 1000; +/** Keeps a pathological source from writing an unbounded status document. */ +const MAX_RECORDED_SKIPPED_SKILLS = 20; +/** Shared cap for skipped-skill and successful-skill validation warning logs. */ +const MAX_LOGGED_PER_SKILL_WARNINGS = 20; +const SKIP_PATH_MAX = 500; +const SKIP_NAME_MAX = 128; +const SKIP_MESSAGE_MAX = 500; +const VALIDATION_ISSUE_LIMIT = 5; +const VALIDATION_ISSUE_FIELD_MAX = 100; +const VALIDATION_ISSUE_CODE_MAX = 64; +const VALIDATION_ISSUE_MESSAGE_MAX = 250; export const GITHUB_FINE_GRAINED_TOKEN_RECOMMENDATION = 'Use a GitHub fine-grained personal access token scoped to the selected repository with read-only Contents and Metadata permissions.'; @@ -79,6 +92,7 @@ type SyncCounters = { syncedFileCount: number; deletedSkillCount: number; deletedFileCount: number; + skippedSkillCount: number; }; type AssertNotCancelled = () => void; @@ -92,6 +106,7 @@ type DiscoveredSkill = { type UpsertRemoteSkillResult = { skill: ISkill & { _id: Types.ObjectId }; created: boolean; + warnings?: ValidationIssue[]; }; type PreparedRemoteSkill = { @@ -457,19 +472,164 @@ function serializeDate(date: Date): string { return date.toISOString(); } +function redactErrorText(value: string): string { + return value.replace(/Bearer\s+\S+/gi, 'Bearer [redacted]'); +} + +function escapeDiagnosticControlCharacters(value: string): string { + let escaped = ''; + for (const character of value) { + const codePoint = character.charCodeAt(0); + if ( + !( + (codePoint >= 0 && codePoint <= 0x1f) || + (codePoint >= 0x7f && codePoint <= 0x9f) || + codePoint === 0x2028 || + codePoint === 0x2029 + ) + ) { + escaped += character; + continue; + } + switch (character) { + case '\n': + escaped += '\\n'; + break; + case '\r': + escaped += '\\r'; + break; + case '\t': + escaped += '\\t'; + break; + default: + escaped += `\\u${codePoint.toString(16).padStart(4, '0')}`; + } + } + return escaped; +} + +function sanitizeDiagnosticText(value: string): string { + return escapeDiagnosticControlCharacters(redactErrorText(value)); +} + +function truncateText(value: string, maxLength: number): string { + return value.length > maxLength ? `${value.slice(0, maxLength - 1)}…` : value; +} + +function summarizeValidationIssues(issues: unknown): string | undefined { + if (!Array.isArray(issues)) { + return undefined; + } + const summaries: string[] = []; + for (const rawIssue of issues.slice(0, VALIDATION_ISSUE_LIMIT)) { + if (!rawIssue || typeof rawIssue !== 'object') { + continue; + } + const issue = rawIssue as Partial; + if ( + typeof issue.field !== 'string' || + typeof issue.code !== 'string' || + typeof issue.message !== 'string' + ) { + continue; + } + const field = truncateText(sanitizeDiagnosticText(issue.field), VALIDATION_ISSUE_FIELD_MAX); + const code = truncateText(sanitizeDiagnosticText(issue.code), VALIDATION_ISSUE_CODE_MAX); + const message = truncateText( + sanitizeDiagnosticText(issue.message), + VALIDATION_ISSUE_MESSAGE_MAX, + ); + summaries.push(`${field} [${code}]: ${message}`); + } + if (summaries.length === 0) { + return undefined; + } + if (issues.length > VALIDATION_ISSUE_LIMIT) { + summaries.push(`+${issues.length - VALIDATION_ISSUE_LIMIT} more issue(s)`); + } + return summaries.join('; '); +} + function sanitizeError(error: unknown): { code: string; message: string } { if (error instanceof SkillSyncError) { - return { code: error.code, message: error.message }; + return { code: error.code, message: sanitizeDiagnosticText(error.message) }; } if (error instanceof Error) { + const message = sanitizeDiagnosticText(error.message); + const validationError = error as Error & { code?: unknown; issues?: unknown }; + if (validationError.code === 'SKILL_VALIDATION_FAILED') { + const issueSummary = summarizeValidationIssues(validationError.issues); + return { + code: 'SKILL_VALIDATION_FAILED', + message: truncateSkipMessage(issueSummary ? `${message}: ${issueSummary}` : message), + }; + } return { code: 'SYNC_FAILED', - message: error.message.replace(/Bearer\s+\S+/gi, 'Bearer [redacted]'), + message, }; } return { code: 'SYNC_FAILED', message: 'Unknown skill sync failure' }; } +/** + * Failures that say nothing more in this run can succeed: the lock is gone, or + * GitHub is refusing every request. They abort the source instead of being + * charged to the skill that happened to hit them first. Everything else is + * scoped to one skill and only skips that skill. + */ +const SOURCE_FATAL_ERROR_CODES = new Set([ + 'SYNC_LOCK_LOST', + 'GITHUB_AUTH_FAILED', + 'GITHUB_RATE_LIMITED', + 'GITHUB_REQUEST_FAILED', + 'SYNC_ROLLBACK_FAILED', +]); + +/** + * A skill that fails and rolls back cleanly is just a skipped skill. One whose + * rollback also fails leaves a half-written mirror behind, and reporting that + * as `partial` alongside the skills that did publish would bury it, so it ends + * the source instead. + */ +function makeRollbackFailure(error: unknown): SkillSyncError { + return new SkillSyncError( + 'SYNC_ROLLBACK_FAILED', + `Rollback failed after: ${sanitizeError(error).message}`, + ); +} + +function makeStaleDeletionFailure(error: unknown): SkillSyncError { + return new SkillSyncError( + 'SYNC_ROLLBACK_FAILED', + `Stale mirror deletion failed: ${sanitizeError(error).message}`, + ); +} + +function isSourceFatalError(error: unknown): boolean { + return error instanceof SkillSyncError && SOURCE_FATAL_ERROR_CODES.has(error.code); +} + +function truncateSkipMessage(message: string): string { + const sanitized = escapeDiagnosticControlCharacters(message); + return sanitized.length > SKIP_MESSAGE_MAX + ? `${sanitized.slice(0, SKIP_MESSAGE_MAX - 1)}…` + : sanitized; +} + +function truncateSkipPath(path: string): string { + const sanitized = escapeDiagnosticControlCharacters(path); + return sanitized.length > SKIP_PATH_MAX ? `${sanitized.slice(0, SKIP_PATH_MAX - 1)}…` : sanitized; +} + +function truncateSkipName(name: string | undefined): string | undefined { + if (!name) { + return name; + } + const sanitized = escapeDiagnosticControlCharacters(name); + return sanitized.length > SKIP_NAME_MAX ? `${sanitized.slice(0, SKIP_NAME_MAX - 1)}…` : sanitized; +} + function buildGitHubHeaders(token: string): HeadersInit { return { Accept: 'application/vnd.github+json', @@ -514,9 +674,17 @@ async function githubJson(params: { token: string; pathname: string; }): Promise { - const response = await params.fetchFn(buildGitHubUrl(params.pathname), { - headers: buildGitHubHeaders(params.token), - }); + let response: Response; + try { + response = await params.fetchFn(buildGitHubUrl(params.pathname), { + headers: buildGitHubHeaders(params.token), + }); + } catch { + throw new SkillSyncError( + 'GITHUB_REQUEST_FAILED', + 'GitHub request failed before receiving a response', + ); + } if (response.ok) { return (await response.json()) as T; } @@ -771,6 +939,7 @@ function makeStatusInput(params: { errorCode?: string; errorMessage?: string; counts?: Partial; + skippedSkills?: ISkillSyncSkippedSkill[]; }): SkillSyncStatusInput { return { provider: PROVIDER, @@ -790,6 +959,8 @@ function makeStatusInput(params: { syncedFileCount: params.counts?.syncedFileCount ?? 0, deletedSkillCount: params.counts?.deletedSkillCount ?? 0, deletedFileCount: params.counts?.deletedFileCount ?? 0, + skippedSkillCount: params.counts?.skippedSkillCount ?? 0, + skippedSkills: params.skippedSkills, }; } @@ -890,7 +1061,7 @@ async function commitRemoteSkill( update: prepared.update, }); if (result.status === 'updated') { - return { skill: result.skill, created: false }; + return { skill: result.skill, created: false, warnings: result.warnings }; } if (result.status === 'conflict') { throw new SkillSyncError( @@ -904,7 +1075,7 @@ async function commitRemoteSkill( ); } const created = await deps.createSkill(prepared.createInput); - return { skill: created.skill, created: true }; + return { skill: created.skill, created: true, warnings: created.warnings }; } /** @@ -927,7 +1098,10 @@ function hasExternalSkillEdit(before: ISkill, after: ISkill): boolean { async function commitExistingRemoteSkillAfterFileSync( deps: GitHubSkillSyncDeps, prepared: PreparedExistingRemoteSkill, - options: { forceCommit?: boolean } = {}, + options: { + forceCommit?: boolean; + logSkillWarnings: (name: string, warnings: ValidationIssue[] | undefined) => void; + }, ): Promise { const refreshed = await deps.getSkillById(prepared.existing._id); if (!refreshed) { @@ -945,7 +1119,9 @@ async function commitExistingRemoteSkillAfterFileSync( if (!options.forceCommit && !hasRemoteSkillDefinitionChanged(prepared.update, refreshed)) { return { skill: refreshed, created: false }; } - return commitRemoteSkill(deps, { ...prepared, existing: refreshed }); + const result = await commitRemoteSkill(deps, { ...prepared, existing: refreshed }); + options.logSkillWarnings(result.skill.name, result.warnings); + return result; } async function cleanupFile(deps: GitHubSkillSyncDeps, file: StoredSkillFileRef): Promise { @@ -1034,17 +1210,23 @@ async function cleanupStoredFiles(params: { deps: GitHubSkillSyncDeps; files: StoredSkillFileRef[]; logMessage: string; + throwOnError?: boolean; }): Promise { const seen = new Set(); + const cleanupErrors: unknown[] = []; 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), - ); + await cleanupFile(params.deps, file).catch((cleanupError) => { + cleanupErrors.push(cleanupError); + logger.error(params.logMessage, cleanupError); + }); + } + if (params.throwOnError && cleanupErrors.length > 0) { + throw cleanupErrors[0]; } } @@ -1071,6 +1253,7 @@ async function restoreExistingSkillFiles(params: { deps, files: savedFiles, logMessage: '[GitHubSkillSync] Failed to clean up rolled-back synced file:', + throwOnError: true, }); } @@ -1205,26 +1388,51 @@ function getMirrorNameKey(params: { return `${params.tenantId ?? ''}:${params.author}:${params.name ?? ''}`; } -function assertNoDuplicatePreparedSkillNames( +/** + * Two upstream skills claiming one mirror name have no non-arbitrary winner, so + * every member of the colliding group is dropped rather than letting tree order + * decide which one the mirror ends up holding. Skills with unique names are + * unaffected: one bad pair no longer costs the rest of the repository. + */ +function partitionDuplicatePreparedSkillNames( source: SkillSyncGitHubSourceConfig, preparedSkills: PreparedDiscoveredSkill[], -): void { +): { unique: PreparedDiscoveredSkill[]; duplicates: PreparedDiscoveredSkill[] } { const sourceTenantId = source.tenantId ?? undefined; - const seen = new Map(); - for (const { discovered, prepared } of preparedSkills) { + const groups = new Map(); + for (const entry of preparedSkills) { const key = getMirrorNameKey({ tenantId: sourceTenantId, - author: prepared.createInput.author.toString(), - name: prepared.createInput.name, + author: entry.prepared.createInput.author.toString(), + name: entry.prepared.createInput.name, }); - if (seen.has(key)) { - throw new SkillSyncError( - 'DUPLICATE_SKILL_NAME', - `GitHub source "${source.id}" contains multiple skills named "${prepared.createInput.name}"`, - ); + const group = groups.get(key); + if (group) { + group.push(entry); + continue; } - seen.set(key, discovered.rootPath); + groups.set(key, [entry]); } + const unique: PreparedDiscoveredSkill[] = []; + const duplicates: PreparedDiscoveredSkill[] = []; + for (const group of groups.values()) { + if (group.length === 1) { + unique.push(group[0]); + continue; + } + duplicates.push(...group); + } + return { unique, duplicates }; +} + +function makeDuplicateNameError( + source: SkillSyncGitHubSourceConfig, + name: string | undefined, +): SkillSyncError { + return new SkillSyncError( + 'DUPLICATE_SKILL_NAME', + `GitHub source "${source.id}" contains multiple skills named "${name}"`, + ); } async function deleteNameConflictingStaleSkill(params: { @@ -1258,7 +1466,11 @@ async function deleteNameConflictingStaleSkill(params: { const { deletedFileCount, deletedSkill } = await deleteSyncedSkillForRestore( params.deps, staleSkill, - ); + ).catch((error) => { + /* deleteSkill can remove the skill row before a later file deletion fails. + The caller has no complete journal to restore from in that case. */ + throw makeStaleDeletionFailure(error); + }); const staleSkillId = staleSkill._id.toString(); return { @@ -1342,9 +1554,10 @@ async function syncSkillFiles(params: { tenantId: skill.tenantId, }); } catch (error) { - await cleanupFile(deps, savedFile).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 makeRollbackFailure(error); + }); throw error; } syncedFileCount++; @@ -1375,13 +1588,18 @@ async function deleteSyncedSkill( ): Promise { const files = await deps.listSkillFiles(skill._id); let deletedFiles = 0; + const cleanupErrors: unknown[] = []; for (const file of files) { - await cleanupFile(deps, file).catch((cleanupError) => - logger.error('[GitHubSkillSync] Failed to clean up mirrored skill file:', cleanupError), - ); + await cleanupFile(deps, file).catch((cleanupError) => { + cleanupErrors.push(cleanupError); + logger.error('[GitHubSkillSync] Failed to clean up mirrored skill file:', cleanupError); + }); deletedFiles++; } await deps.deleteSkill(skill._id.toString()); + if (cleanupErrors.length > 0) { + throw cleanupErrors[0]; + } return deletedFiles; } @@ -1429,6 +1647,14 @@ async function syncSource(params: { }): Promise { const { deps, source, fetchFn, assertNotCancelled } = params; const startedAt = new Date(); + const counts: SyncCounters = { + syncedSkillCount: 0, + syncedFileCount: 0, + deletedSkillCount: 0, + deletedFileCount: 0, + skippedSkillCount: 0, + }; + const skippedSkills: ISkillSyncSkippedSkill[] = []; await deps.upsertStatus(makeStatusInput({ source, status: 'running', startedAt })); try { assertNotCancelled(); @@ -1463,63 +1689,196 @@ async function syncSource(params: { } return existingSyncedSkills; }; - const counts: SyncCounters = { - syncedSkillCount: 0, - syncedFileCount: 0, - deletedSkillCount: 0, - deletedFileCount: 0, + let loggedPerSkillWarningCount = 0; + let suppressedSkippedWarningCount = 0; + let suppressedValidationWarningCount = 0; + /** + * Non-blocking validation issues have no user-facing surface on a background + * sync. Keep them visible without allowing a large source to amplify logs. + */ + const logSkillWarnings = (name: string, warnings: ValidationIssue[] | undefined): void => { + if (!warnings?.length) { + return; + } + if (loggedPerSkillWarningCount >= MAX_LOGGED_PER_SKILL_WARNINGS) { + suppressedValidationWarningCount++; + return; + } + const summary = summarizeValidationIssues(warnings); + if (!summary) { + return; + } + logger.warn( + `[GitHubSkillSync] Skill "${truncateSkipName(name)}" synced with warnings: ${truncateSkipMessage(summary)}`, + ); + loggedPerSkillWarningCount++; + }; + const logSuppressedPerSkillWarningSummaries = (): void => { + if (suppressedSkippedWarningCount > 0) { + logger.warn( + `[GitHubSkillSync] Source "${source.id}" suppressed ${suppressedSkippedWarningCount} additional skipped skill warning(s)`, + ); + } + if (suppressedValidationWarningCount > 0) { + logger.warn( + `[GitHubSkillSync] Source "${source.id}" suppressed ${suppressedValidationWarningCount} additional synced skill validation warning(s)`, + ); + } + }; + /** + * Charges one skill's failure to that skill and lets the run continue. + * Source-level failures are rethrown so the whole source still fails fast + * instead of being reported as a long list of skipped skills. + */ + const recordSkippedSkill = ({ + path, + name, + error, + }: { + path: string; + name?: string; + error: unknown; + }): void => { + if (isSourceFatalError(error)) { + throw error; + } + const sanitized = sanitizeError(error); + counts.skippedSkillCount++; + if (loggedPerSkillWarningCount < MAX_LOGGED_PER_SKILL_WARNINGS) { + logger.warn( + `[GitHubSkillSync] Source "${source.id}" skipped "${truncateSkipPath(path)}": ${truncateSkipMessage(sanitized.message)}`, + ); + loggedPerSkillWarningCount++; + } else { + suppressedSkippedWarningCount++; + } + if (skippedSkills.length >= MAX_RECORDED_SKIPPED_SKILLS) { + return; + } + skippedSkills.push({ + path: truncateSkipPath(path), + name: truncateSkipName(name), + errorCode: sanitized.code, + errorMessage: truncateSkipMessage(sanitized.message), + }); }; const syncedAt = new Date(); const preparedSkills: PreparedDiscoveredSkill[] = []; + let canReconcileStaleSkills = true; + /* Built from everything discovered upstream, not just what prepared + cleanly: a skill that failed to prepare is still present in the + repository, so it must not look stale or like a rename target. */ + const discoveredUpstreamIds = new Set( + discoveredSkills.map((discovered) => makeUpstreamId(source, discovered.rootPath)), + ); for (const discovered of discoveredSkills) { assertNotCancelled(); - assertGitHubSkillPackageManifest(discovered); - const skillMdPath = getSkillMdPath(discovered); - const skillMdBuffer = await fetchBlob({ - fetchFn, - token, - source, - sha: discovered.skillMd.sha, - }); - assertNotCancelled(); - assertGitHubBufferSize(skillMdBuffer, skillMdPath); - const prepared = await prepareRemoteSkill({ - deps, - source, - discovered, - skillMdContent: skillMdBuffer.toString('utf-8'), - commitSha: commit.sha, - syncedAt, - }); - preparedSkills.push({ discovered, prepared }); + try { + assertGitHubSkillPackageManifest(discovered); + const skillMdPath = getSkillMdPath(discovered); + const skillMdBuffer = await fetchBlob({ + fetchFn, + token, + source, + sha: discovered.skillMd.sha, + }); + assertNotCancelled(); + assertGitHubBufferSize(skillMdBuffer, skillMdPath); + const prepared = await prepareRemoteSkill({ + deps, + source, + discovered, + skillMdContent: skillMdBuffer.toString('utf-8'), + commitSha: commit.sha, + syncedAt, + }); + preparedSkills.push({ discovered, prepared }); + } catch (error) { + /* Until preparation succeeds, a moved skill cannot be matched to the + mirror that still carries its old upstream id. Keep stale mirrors + for this run rather than deleting a last-known-good moved skill. */ + canReconcileStaleSkills = false; + seenUpstreamIds.add(makeUpstreamId(source, discovered.rootPath)); + recordSkippedSkill({ path: discovered.rootPath, error }); + } } - const discoveredUpstreamIds = new Set( - preparedSkills.map(({ discovered }) => makeUpstreamId(source, discovered.rootPath)), - ); - assertNoDuplicatePreparedSkillNames(source, preparedSkills); + /** + * A moved skill's mirror still carries its old upstream id until the update + * lands, and only the new path is marked as seen. Marking the old id keeps + * the published copy in place whenever the new one does not replace it, so + * the reconcile pass cannot read it as stale. + */ + const markMovedMirrorAsSeen = async ( + prepared: PreparedRemoteSkill, + ): Promise<(ISkill & { _id: Types.ObjectId }) | null> => { + if (prepared.existing || !canReconcileStaleSkills) { + return null; + } + const movedExisting = findMovedSourceSkill({ + source, + prepared, + existingSyncedSkills: await getExistingSyncedSkills(), + excludedUpstreamIds: discoveredUpstreamIds, + }); + const movedUpstreamId = movedExisting + ? getSourceMetadataString(movedExisting, 'upstreamId') + : undefined; + if (movedUpstreamId) { + seenUpstreamIds.add(movedUpstreamId); + } + return movedExisting; + }; + + const { unique, duplicates } = partitionDuplicatePreparedSkillNames(source, preparedSkills); + for (const { discovered, prepared } of duplicates) { + seenUpstreamIds.add(makeUpstreamId(source, discovered.rootPath)); + /* A duplicate never reaches `syncPreparedSkill`, so without this its + moved mirror goes unmarked and is reconciled away even though nothing + was published to replace it. */ + const movedMirror = await markMovedMirrorAsSeen(prepared); + if (!prepared.existing && !movedMirror) { + /* A duplicate with a new identity can be a moved and renamed skill. + Without an identity or name match, preserve unmatched stale mirrors + because one may be its last-known-good copy. */ + canReconcileStaleSkills = false; + } + recordSkippedSkill({ + path: discovered.rootPath, + name: prepared.createInput.name, + error: makeDuplicateNameError(source, prepared.createInput.name), + }); + } const orderedPreparedSkills = orderPreparedSkillsForSafeStaleDeletes({ source, - preparedSkills, + preparedSkills: unique, existingSyncedSkills: await getExistingSyncedSkills(), discoveredUpstreamIds, }); - for (const { discovered, prepared } of orderedPreparedSkills) { - assertNotCancelled(); - const movedExisting = prepared.existing - ? null - : findMovedSourceSkill({ - source, - prepared, - existingSyncedSkills: await getExistingSyncedSkills(), - excludedUpstreamIds: discoveredUpstreamIds, - }); + const syncPreparedSkill = async ({ + discovered, + prepared, + }: PreparedDiscoveredSkill): Promise => { + if (!prepared.existing && !canReconcileStaleSkills) { + const ambiguousMovedMirror = findMovedSourceSkill({ + source, + prepared, + existingSyncedSkills: await getExistingSyncedSkills(), + excludedUpstreamIds: discoveredUpstreamIds, + }); + if (ambiguousMovedMirror) { + throw new SkillSyncError( + 'SKILL_MOVE_AMBIGUOUS', + `Skill "${prepared.createInput.name}" may have moved, but another skill could not be prepared`, + ); + } + } + const movedExisting = await markMovedMirrorAsSeen(prepared); const effectivePrepared: PreparedRemoteSkill = movedExisting ? { ...prepared, existing: movedExisting } : prepared; - seenUpstreamIds.add(makeUpstreamId(source, discovered.rootPath)); if (effectivePrepared.existing) { // Check for an external edit before mutating files, so a concurrently // edited skill fails fast without leaving its bundled files partially @@ -1557,7 +1916,7 @@ async function syncSource(params: { assertNotCancelled, journal, }); - if (prepared.existing) { + if (prepared.existing && canReconcileStaleSkills) { staleConflictCleanup = await deleteNameConflictingStaleSkill({ deps, source, @@ -1576,30 +1935,41 @@ async function syncSource(params: { ...effectivePrepared, existing: effectivePrepared.existing, }, - { forceCommit: fileCounts.syncedFileCount > 0 || fileCounts.deletedFileCount > 0 }, + { + forceCommit: fileCounts.syncedFileCount > 0 || fileCounts.deletedFileCount > 0, + logSkillWarnings, + }, ); } catch (error) { + let rollbackFailed = false; await restoreExistingSkillFiles({ deps, skill: effectivePrepared.existing, previousFiles, savedFiles: journal.savedFiles, - }).catch((cleanupError) => + }).catch((cleanupError) => { + rollbackFailed = true; logger.error( '[GitHubSkillSync] Failed to restore existing skill files after sync failure:', cleanupError, - ), - ); + ); + }); if (staleConflictCleanup?.deletedSkill) { await restoreDeletedSyncedSkill(deps, staleConflictCleanup.deletedSkill).catch( - (cleanupError) => + (cleanupError) => { logger.error( - '[GitHubSkillSync] Failed to restore stale mirrored skill after sync failure:', + '[GitHubSkillSync] Failed to recreate stale mirrored skill after sync failure:', cleanupError, - ), + ); + }, ); + /* deleteSkill removes the original id from agent allowlists and + deletes every ACL entry. Recreating the row recovers its data, + but cannot restore that dependent state, so this is never a + complete rollback and the source must fail visibly. */ + rollbackFailed = true; } - throw error; + throw rollbackFailed ? makeRollbackFailure(error) : error; } await cleanupStoredFiles({ deps, @@ -1612,7 +1982,7 @@ async function syncSource(params: { counts.syncedSkillCount++; counts.syncedFileCount += fileCounts.syncedFileCount; counts.deletedFileCount += fileCounts.deletedFileCount; - continue; + return; } const upserted = await commitRemoteSkill(deps, effectivePrepared); @@ -1629,17 +1999,53 @@ async function syncSource(params: { assertNotCancelled, }); await ensurePublicViewer(deps, skill._id); + logSkillWarnings(skill.name, upserted.warnings); counts.syncedSkillCount++; counts.syncedFileCount += fileCounts.syncedFileCount; counts.deletedFileCount += fileCounts.deletedFileCount; } catch (error) { - await deleteSyncedSkill(deps, skill).catch((cleanupError) => - logger.error( - '[GitHubSkillSync] Failed to roll back partially synced skill:', - cleanupError, - ), - ); - throw error; + const rolledBack = await deleteSyncedSkill(deps, skill) + .then(() => true) + .catch((cleanupError) => { + logger.error( + '[GitHubSkillSync] Failed to roll back partially synced skill:', + cleanupError, + ); + return false; + }); + throw rolledBack ? error : makeRollbackFailure(error); + } + }; + + for (const entry of orderedPreparedSkills) { + assertNotCancelled(); + /* Marked as seen before the attempt: a skill that fails here is still + present upstream, so the reconcile pass below must not mirror-delete + a copy that a later run can repair. */ + seenUpstreamIds.add(makeUpstreamId(source, entry.discovered.rootPath)); + try { + await syncPreparedSkill(entry); + } catch (error) { + if ( + !entry.prepared.existing && + !findMovedSourceSkill({ + source, + prepared: entry.prepared, + existingSyncedSkills: await getExistingSyncedSkills(), + excludedUpstreamIds: discoveredUpstreamIds, + }) + ) { + /* A new identity can be a moved and renamed skill that name-based + matching cannot associate with its old mirror. If it fails after + preparation, preserve stale mirrors because the old upstream id + is unknown and may be the last-known-good copy. */ + canReconcileStaleSkills = false; + } + recordSkippedSkill({ + path: entry.discovered.rootPath, + name: entry.prepared.createInput.name, + error, + }); } } @@ -1662,20 +2068,44 @@ async function syncSource(params: { skill.sourceMetadata && typeof skill.sourceMetadata.upstreamId === 'string' ? skill.sourceMetadata.upstreamId : ''; - if (seenUpstreamIds.has(upstreamId)) { + if (!canReconcileStaleSkills || seenUpstreamIds.has(upstreamId)) { continue; } counts.deletedFileCount += await deleteSyncedSkill(deps, skill); counts.deletedSkillCount++; } + if (counts.skippedSkillCount === 0) { + logSuppressedPerSkillWarningSummaries(); + return deps.upsertStatus( + makeStatusInput({ + source, + status: 'succeeded', + startedAt, + finishedAt: new Date(), + counts, + }), + ); + } + /* Nothing published and something skipped means the source produced no + usable mirror at all, which is a failure however it is spelled. The + first skip carries the reason so the status is actionable. */ + const publishedNothing = counts.syncedSkillCount === 0; + const firstSkip = skippedSkills[0]; + logSuppressedPerSkillWarningSummaries(); + logger.warn( + `[GitHubSkillSync] Source "${source.id}" synced ${counts.syncedSkillCount} skill(s) and skipped ${counts.skippedSkillCount}`, + ); return deps.upsertStatus( makeStatusInput({ source, - status: 'succeeded', + status: publishedNothing ? 'failed' : 'partial', startedAt, finishedAt: new Date(), counts, + skippedSkills, + errorCode: publishedNothing ? firstSkip?.errorCode : undefined, + errorMessage: publishedNothing ? firstSkip?.errorMessage : undefined, }), ); } catch (error) { @@ -1687,6 +2117,14 @@ async function syncSource(params: { status: 'failed', startedAt, finishedAt: new Date(), + counts: { + syncedSkillCount: 0, + syncedFileCount: 0, + deletedSkillCount: 0, + deletedFileCount: 0, + skippedSkillCount: counts.skippedSkillCount, + }, + skippedSkills: skippedSkills.length > 0 ? skippedSkills : undefined, errorCode: sanitized.code, errorMessage: sanitized.message, }), @@ -1781,6 +2219,8 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps): GitHubSk syncedFileCount: stored?.syncedFileCount ?? 0, deletedSkillCount: stored?.deletedSkillCount ?? 0, deletedFileCount: stored?.deletedFileCount ?? 0, + skippedSkillCount: stored?.skippedSkillCount ?? 0, + skippedSkills: stored?.skippedSkills, createdAt: stored?.createdAt, updatedAt: stored?.updatedAt, } satisfies ISkillSyncStatus & { credentialPresent: boolean }; diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index c86359e966..97ca5a56a8 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -57,6 +57,7 @@ function statusFromConfig( syncedFileCount: 0, deletedSkillCount: 0, deletedFileCount: 0, + skippedSkillCount: 0, errorCode: undefined, errorMessage: undefined, startedAt: undefined, diff --git a/packages/data-provider/src/types/skills.ts b/packages/data-provider/src/types/skills.ts index 487edc29dc..ba202b9dd8 100644 --- a/packages/data-provider/src/types/skills.ts +++ b/packages/data-provider/src/types/skills.ts @@ -38,11 +38,25 @@ export type SkillSource = 'inline' | 'deployment' | 'github' | 'notion'; */ export type SkillFileCategory = 'script' | 'reference' | 'asset' | 'other'; +/** Nested object inside a structured frontmatter key. */ +export type SkillFrontmatterObject = { [key: string]: SkillFrontmatterValue | undefined }; + /** - * Allowed value types inside a skill's YAML frontmatter. - * Kept strict so callers cannot slip arbitrary `unknown` payloads through the API. + * Allowed value types inside a skill's YAML frontmatter. Scalars cover the + * documented keys; nested arrays and objects describe the structured ones + * (`hooks`, `metadata`, `references`), which real `SKILL.md` files write as a + * list, a list of objects, or a map. + * + * Still no `unknown` or `any`: the payload is JSON-safe by construction, and + * the server bounds depth, string length and array size when validating it. */ -export type SkillFrontmatterValue = string | number | boolean | string[] | null; +export type SkillFrontmatterValue = + | string + | number + | boolean + | null + | SkillFrontmatterValue[] + | SkillFrontmatterObject; /** * Structured YAML frontmatter for a skill. All keys are optional on the wire @@ -106,8 +120,8 @@ export type TSkillWarning = { * - `description` is the "when to use this skill" sentence. Highest-leverage * field for trigger accuracy; a short/vague one causes undertriggering. * - `frontmatter` is the structured YAML bag minus `name`/`description` - * (those live as top-level columns). Validated strictly against a known - * key set server-side. + * (those live as top-level columns). Known keys receive value validation; + * unknown keys are retained and reported as non-blocking warnings. * - `source`/`sourceMetadata` identify whether the row is user-authored, * deployment-provided, or mirrored from an external source such as GitHub. */ @@ -215,11 +229,20 @@ export type TGitHubSkillSyncCredentialSummary = { createdAt?: string; }; +/** One upstream skill a sync run dropped, with the reason it was dropped. */ +export type TGitHubSkillSyncSkippedSkill = { + path: string; + name?: string; + errorCode: string; + errorMessage: string; +}; + export type TGitHubSkillSyncSourceStatus = { provider: 'github'; sourceId: string; tenantId?: string; - status: 'idle' | 'running' | 'succeeded' | 'failed' | 'skipped'; + /** `partial`: some skills published, others were skipped (see `skippedSkills`). */ + status: 'idle' | 'running' | 'succeeded' | 'partial' | 'failed' | 'skipped'; credentialKey?: string; credentialPresent: boolean; owner?: string; @@ -236,6 +259,8 @@ export type TGitHubSkillSyncSourceStatus = { syncedFileCount: number; deletedSkillCount: number; deletedFileCount: number; + skippedSkillCount: number; + skippedSkills?: TGitHubSkillSyncSkippedSkill[]; updatedAt?: string; createdAt?: string; }; diff --git a/packages/data-schemas/src/index.ts b/packages/data-schemas/src/index.ts index a7ac5a80ab..a860a1b682 100644 --- a/packages/data-schemas/src/index.ts +++ b/packages/data-schemas/src/index.ts @@ -22,6 +22,8 @@ export { validateRelativePath, inferSkillFileCategory, validateSkillFrontmatter, + getCanonicalSkillFrontmatterKey, + normalizeSkillFrontmatterKeys, validateSkillDescription, deriveStructuredFrontmatterFields, AUDIT_SCHEMA_VERSION, diff --git a/packages/data-schemas/src/methods/index.ts b/packages/data-schemas/src/methods/index.ts index f3b71cda70..1683489a8d 100644 --- a/packages/data-schemas/src/methods/index.ts +++ b/packages/data-schemas/src/methods/index.ts @@ -77,6 +77,8 @@ import { validateSkillBody, validateRelativePath, validateSkillFrontmatter, + getCanonicalSkillFrontmatterKey, + normalizeSkillFrontmatterKeys, validateSkillDescription, deriveStructuredFrontmatterFields, inferSkillFileCategory, @@ -136,6 +138,8 @@ export { validateSkillBody, validateRelativePath, validateSkillFrontmatter, + getCanonicalSkillFrontmatterKey, + normalizeSkillFrontmatterKeys, validateSkillDescription, deriveStructuredFrontmatterFields, inferSkillFileCategory, diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 0cd12f9e9d..c71bafc71c 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -9,9 +9,12 @@ import { PermissionBits, } from 'librechat-data-provider'; import { + partitionIssues, validateSkillName, validateSkillDescription, validateSkillFrontmatter, + getCanonicalSkillFrontmatterKey, + normalizeSkillFrontmatterKeys, validateAlwaysApply, validateRelativePath, inferSkillFileCategory, @@ -257,6 +260,38 @@ describe('skill validation helpers', () => { }); describe('validateSkillFrontmatter', () => { + it('canonicalizes recognized keys without rewriting unknown keys', () => { + expect(getCanonicalSkillFrontmatterKey('Allowed-Tools')).toBe('allowed-tools'); + expect(getCanonicalSkillFrontmatterKey('ALWAYSAPPLY')).toBe('alwaysApply'); + expect(getCanonicalSkillFrontmatterKey('customConfig')).toBeUndefined(); + expect( + normalizeSkillFrontmatterKeys({ + 'Allowed-Tools': ['execute_code'], + customConfig: { mode: 'strict' }, + }), + ).toEqual({ + frontmatter: { + 'allowed-tools': ['execute_code'], + customConfig: { mode: 'strict' }, + }, + }); + }); + + it('rejects case-colliding recognized keys', () => { + const frontmatter = { + 'allowed-tools': ['read_file'], + 'Allowed-Tools': ['execute_code'], + }; + + expect(normalizeSkillFrontmatterKeys(frontmatter)).toEqual({ + error: + 'Recognized frontmatter keys "allowed-tools" and "Allowed-Tools" both resolve to "allowed-tools"', + }); + expect(validateSkillFrontmatter(frontmatter)).toEqual([ + expect.objectContaining({ field: 'frontmatter', code: 'DUPLICATE_KEY' }), + ]); + }); + it('accepts an undefined or empty frontmatter', () => { expect(validateSkillFrontmatter(undefined)).toEqual([]); expect(validateSkillFrontmatter(null)).toEqual([]); @@ -270,9 +305,91 @@ describe('skill validation helpers', () => { expect(validateSkillFrontmatter([]).some((i) => i.code === 'INVALID_TYPE')).toBe(true); }); - it('rejects unknown keys in strict mode', () => { + it('warns about unknown keys instead of rejecting them', () => { const issues = validateSkillFrontmatter({ 'not-a-real-key': 'value' }); - expect(issues.some((i) => i.code === 'UNKNOWN_KEY')).toBe(true); + expect(issues).toEqual([ + expect.objectContaining({ + field: 'frontmatter.not-a-real-key', + code: 'UNKNOWN_KEY', + severity: 'warning', + }), + ]); + expect(partitionIssues(issues).errors).toEqual([]); + }); + + it('still bounds the value of an unknown key', () => { + /* The key is tolerated, the payload is not: an unrecognized key is + persisted, so it stays inside the same limits as every other key. */ + const deep = { a: { b: { c: { d: { e: { f: 'too deep' } } } } } }; + const issues = validateSkillFrontmatter({ 'not-a-real-key': deep }); + + expect(issues.some((i) => i.code === 'UNKNOWN_KEY' && i.severity === 'warning')).toBe(true); + expect( + partitionIssues(issues).errors.some( + (i) => i.code === 'INVALID_SHAPE' && i.field === 'frontmatter.not-a-real-key', + ), + ).toBe(true); + }); + + it('rejects non-plain object values under unknown keys', () => { + const issues = validateSkillFrontmatter({ created: new Date('2026-08-11T00:00:00.000Z') }); + + expect(issues.some((i) => i.code === 'UNKNOWN_KEY' && i.severity === 'warning')).toBe(true); + expect( + partitionIssues(issues).errors.some( + (i) => i.code === 'INVALID_SHAPE' && i.field === 'frontmatter.created', + ), + ).toBe(true); + }); + + it('rejects NUL characters in unknown key names before persistence', () => { + const issues = validateSkillFrontmatter({ ['custom\u0000key']: 'value' }); + + expect(partitionIssues(issues).errors).toEqual([ + expect.objectContaining({ + field: 'frontmatter', + code: 'INVALID_KEY', + }), + ]); + expect(issues.some((i) => i.code === 'UNKNOWN_KEY')).toBe(false); + }); + + it('rejects object property names that Mongoose cannot persist at any depth', () => { + for (const key of ['__proto__', 'constructor', 'prototype']) { + const topLevel = validateSkillFrontmatter(Object.fromEntries([[key, 'value']])); + const nested = validateSkillFrontmatter({ + metadata: Object.fromEntries([[key, 'value']]), + }); + + expect(partitionIssues(topLevel).errors).toEqual([ + expect.objectContaining({ field: 'frontmatter', code: 'INVALID_KEY' }), + ]); + expect(partitionIssues(nested).errors).toEqual([ + expect.objectContaining({ field: 'frontmatter.metadata', code: 'INVALID_KEY' }), + ]); + } + }); + + it('accepts the references key in every shape real SKILL.md files use', () => { + expect(validateSkillFrontmatter({ references: ['workers', 'pages', 'd1'] })).toEqual([]); + expect(validateSkillFrontmatter({ references: 'references/api.md' })).toEqual([]); + expect( + validateSkillFrontmatter({ + references: [{ path: 'references/api.md', description: 'API surface' }], + }), + ).toEqual([]); + expect( + validateSkillFrontmatter({ references: { workers: 'references/workers.md' } }), + ).toEqual([]); + }); + + it('rejects a references value with excessive nesting', () => { + const deep = { a: { b: { c: { d: { e: { f: 'too deep' } } } } } }; + expect( + validateSkillFrontmatter({ references: deep }).some( + (i) => i.code === 'INVALID_SHAPE' && i.field === 'frontmatter.references', + ), + ).toBe(true); }); it('accepts known keys with correct types', () => { @@ -415,6 +532,9 @@ describe('skill validation helpers', () => { /* Empty string → not extracted; an explicit empty array is the author's way to say "no extras". */ expect(deriveStructuredFrontmatterFields({ 'allowed-tools': '' })).toEqual({}); + expect(deriveStructuredFrontmatterFields({ 'Allowed-Tools': 'execute_code' })).toEqual({ + allowedTools: ['execute_code'], + }); }); it('passes through array allowed-tools, dropping non-string entries', () => { @@ -469,12 +589,100 @@ describe('Skill CRUD methods', () => { ]); }); - it('rejects frontmatter with unknown keys (strict mode)', async () => { + it('creates the skill and warns when frontmatter carries an unknown key', async () => { + /* One unrecognized key in one SKILL.md must not fail the skill: the GitHub + sync runner marks the whole source failed on a validation error, so a + stray key used to block every other skill in the repository. */ + const { skill, warnings } = await methods.createSkill( + makeSkillInput({ + name: 'unknown-key-frontmatter', + frontmatter: { name: 'unknown-key-frontmatter', 'bogus-key': 'nope' }, + }), + ); + expect(skill._id).toBeDefined(); + expect(skill.frontmatter).toMatchObject({ 'bogus-key': 'nope' }); + expect(warnings).toEqual([ + expect.objectContaining({ + field: 'frontmatter.bogus-key', + code: 'UNKNOWN_KEY', + severity: 'warning', + }), + ]); + }); + + it('canonicalizes recognized frontmatter variants before persistence and derivation', async () => { + const { skill, warnings } = await methods.createSkill( + makeSkillInput({ + name: 'case-variant-frontmatter', + frontmatter: { + name: 'case-variant-frontmatter', + 'Allowed-Tools': ['execute_code'], + 'User-Invocable': false, + }, + }), + ); + + expect(skill.frontmatter).toMatchObject({ + 'allowed-tools': ['execute_code'], + 'user-invocable': false, + }); + expect(skill.frontmatter).not.toHaveProperty('Allowed-Tools'); + expect(skill.allowedTools).toEqual(['execute_code']); + expect(skill.userInvocable).toBe(false); + expect(warnings).toEqual([]); + }); + + it('rejects case-colliding recognized frontmatter keys', async () => { await expect( methods.createSkill( makeSkillInput({ - name: 'strict-frontmatter', - frontmatter: { 'bogus-key': 'nope' }, + name: 'case-collision-frontmatter', + frontmatter: { + 'allowed-tools': ['read_file'], + 'Allowed-Tools': ['execute_code'], + }, + }), + ), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + }); + + it('creates a skill whose frontmatter carries a references list', async () => { + const { skill, warnings } = await methods.createSkill( + makeSkillInput({ + name: 'references-frontmatter', + frontmatter: { + name: 'references-frontmatter', + description: 'A small demo skill used in tests.', + references: ['workers', 'pages', 'd1'], + }, + }), + ); + expect(skill.frontmatter).toMatchObject({ references: ['workers', 'pages', 'd1'] }); + expect(warnings).toEqual([]); + }); + + it('still rejects malformed frontmatter', async () => { + await expect( + methods.createSkill( + makeSkillInput({ + name: 'malformed-frontmatter', + frontmatter: { 'user-invocable': 'yes' }, + }), + ), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + await expect( + methods.createSkill( + makeSkillInput({ + name: 'non-object-frontmatter', + frontmatter: 'not an object', + }), + ), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + await expect( + methods.createSkill( + makeSkillInput({ + name: 'deep-hooks-frontmatter', + frontmatter: { hooks: { a: { b: { c: { d: { e: { f: 'too deep' } } } } } } }, }), ), ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 4b0632a336..ae6852cec8 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -236,10 +236,12 @@ export function validateAlwaysApply(alwaysApply: unknown): ValidationIssue[] { /** * Known fields allowed inside a skill's YAML frontmatter. Anything else is - * rejected in strict mode. The list is derived from Anthropic's Agent Skills - * spec plus the fields LibreChat needs to pass through (`name`/`description` - * are duplicated from the top-level columns because real `SKILL.md` files - * include them in their frontmatter block). + * reported as a warning (see `validateSkillFrontmatter`) rather than rejected: + * the frontmatter convention keeps growing, and a single unrecognized key in + * one `SKILL.md` used to fail its whole GitHub sync source. The list is derived + * from Anthropic's Agent Skills spec plus the fields LibreChat needs to pass + * through (`name`/`description` are duplicated from the top-level columns + * because real `SKILL.md` files include them in their frontmatter block). */ const ALLOWED_FRONTMATTER_KEYS = new Set([ 'name', @@ -263,11 +265,42 @@ const ALLOWED_FRONTMATTER_KEYS = new Set([ 'license', 'compatibility', 'metadata', + 'references', ]); +const CANONICAL_FRONTMATTER_KEYS = new Map( + Array.from(ALLOWED_FRONTMATTER_KEYS, (key) => [key.toLowerCase(), key]), +); + +export function getCanonicalSkillFrontmatterKey(key: string): string | undefined { + return CANONICAL_FRONTMATTER_KEYS.get(key.toLowerCase()); +} + +export function normalizeSkillFrontmatterKeys( + frontmatter: Record, +): { frontmatter: Record } | { error: string } { + const normalized = Object.create(null) as Record; + const recognizedKeys = new Map(); + for (const [key, value] of Object.entries(frontmatter)) { + const canonicalKey = getCanonicalSkillFrontmatterKey(key); + if (canonicalKey) { + const previousKey = recognizedKeys.get(canonicalKey); + if (previousKey) { + return { + error: `Recognized frontmatter keys "${previousKey}" and "${key}" both resolve to "${canonicalKey}"`, + }; + } + recognizedKeys.set(canonicalKey, key); + } + normalized[canonicalKey ?? key] = value; + } + return { frontmatter: normalized }; +} + const FRONTMATTER_MAX_STRING = 2000; const FRONTMATTER_MAX_ARRAY = 100; const FRONTMATTER_MAX_DEPTH = 4; +const NON_PERSISTABLE_FRONTMATTER_KEYS = new Set(['__proto__', 'constructor', 'prototype']); type FrontmatterKind = 'string' | 'number' | 'boolean' | 'stringArray'; @@ -294,7 +327,31 @@ const FRONTMATTER_KIND: Record = { }; function isPlainObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value); + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return false; + } + const prototype = Object.getPrototypeOf(value); + return prototype === Object.prototype || prototype === null; +} + +function isValidFrontmatterKey(key: string): boolean { + return !key.includes('\u0000') && !NON_PERSISTABLE_FRONTMATTER_KEYS.has(key); +} + +function containsInvalidFrontmatterKey(value: unknown, depth = 0): boolean { + if (depth > FRONTMATTER_MAX_DEPTH) { + return false; + } + if (Array.isArray(value)) { + return value.some((nestedValue) => containsInvalidFrontmatterKey(nestedValue, depth + 1)); + } + if (!isPlainObject(value)) { + return false; + } + return Object.entries(value).some( + ([key, nestedValue]) => + !isValidFrontmatterKey(key) || containsInvalidFrontmatterKey(nestedValue, depth + 1), + ); } function isStringArray(value: unknown): value is string[] { @@ -338,17 +395,20 @@ function isJsonSafe(value: unknown, depth: number): boolean { return value.every((v) => isJsonSafe(v, depth + 1)); } if (isPlainObject(value)) { - return Object.values(value).every((v) => isJsonSafe(v, depth + 1)); + return Object.entries(value).every( + ([key, nestedValue]) => isValidFrontmatterKey(key) && isJsonSafe(nestedValue, depth + 1), + ); } return false; } /** - * Validate a skill's structured YAML frontmatter. Strict mode: unknown keys - * are rejected so any expansion of the allowed set is an intentional code - * change. Known keys are type-checked against `FRONTMATTER_KIND`; `hooks` and - * `metadata` fall back to a shallow JSON-safety check because their full - * schemas live outside this module. + * Validate a skill's structured YAML frontmatter. Known keys are type-checked + * against `FRONTMATTER_KIND`; `hooks`, `metadata` and `references` fall back to + * a shallow JSON-safety check because their full schemas live outside this + * module. Unknown keys are reported as warnings, not errors: authors regularly + * carry keys from other tooling, and failing the skill for one of them takes + * down every other skill in the same GitHub sync source. */ export function validateSkillFrontmatter(frontmatter: unknown): ValidationIssue[] { if (frontmatter === undefined || frontmatter === null) { @@ -364,14 +424,63 @@ export function validateSkillFrontmatter(frontmatter: unknown): ValidationIssue[ ]; } + const normalized = normalizeSkillFrontmatterKeys(frontmatter); + if ('error' in normalized) { + return [ + { + field: 'frontmatter', + code: 'DUPLICATE_KEY', + message: normalized.error, + }, + ]; + } + const issues: ValidationIssue[] = []; - for (const [key, value] of Object.entries(frontmatter)) { + for (const [key, value] of Object.entries(normalized.frontmatter)) { + if (!isValidFrontmatterKey(key)) { + issues.push({ + field: 'frontmatter', + code: 'INVALID_KEY', + message: 'Frontmatter keys must be persistable object property names', + }); + continue; + } + if (containsInvalidFrontmatterKey(value)) { + issues.push({ + field: `frontmatter.${key}`, + code: 'INVALID_KEY', + message: `"${key}" contains a frontmatter key that cannot be persisted`, + }); + continue; + } if (!ALLOWED_FRONTMATTER_KEYS.has(key)) { issues.push({ field: `frontmatter.${key}`, code: 'UNKNOWN_KEY', - message: `"${key}" is not a recognized frontmatter key`, + severity: 'warning', + message: `"${key}" is not a recognized frontmatter key and is stored as-is`, }); + /* The key is tolerated, its value still is not: an unrecognized key is + persisted, so it stays inside the same depth, array and string bounds + every structured key is held to. */ + if (!isJsonSafe(value, 0)) { + issues.push({ + field: `frontmatter.${key}`, + code: 'INVALID_SHAPE', + message: `"${key}" must be a JSON-safe value (max depth ${FRONTMATTER_MAX_DEPTH}, max string ${FRONTMATTER_MAX_STRING}, max array ${FRONTMATTER_MAX_ARRAY})`, + }); + } + continue; + } + + if (key === 'references') { + if (!isJsonSafe(value, 0)) { + issues.push({ + field: 'frontmatter.references', + code: 'INVALID_SHAPE', + message: `"references" must be a JSON-safe value (max depth ${FRONTMATTER_MAX_DEPTH}, max string ${FRONTMATTER_MAX_STRING})`, + }); + } continue; } @@ -531,6 +640,11 @@ export function deriveStructuredFrontmatterFields( if (!frontmatter || typeof frontmatter !== 'object') { return {}; } + const normalized = normalizeSkillFrontmatterKeys(frontmatter); + if ('error' in normalized) { + return {}; + } + frontmatter = normalized.frontmatter; const derived: { disableModelInvocation?: boolean; userInvocable?: boolean; @@ -1024,6 +1138,13 @@ export function createSkillMethods( } async function createSkill(data: CreateSkillInput): Promise { + const normalizedFrontmatter = isPlainObject(data.frontmatter) + ? normalizeSkillFrontmatterKeys(data.frontmatter) + : undefined; + const frontmatter = + normalizedFrontmatter && 'frontmatter' in normalizedFrontmatter + ? normalizedFrontmatter.frontmatter + : data.frontmatter; /* Parse body's always-apply status once — reused for validation (below) and derivation in `resolveAlwaysApplyFromInput`. Avoids parsing the same YAML frontmatter block twice per create. */ @@ -1034,7 +1155,7 @@ export function createSkillMethods( ...validateSkillDescription(data.description), ...validateSkillBody(data.body), ...validateSkillDisplayTitle(data.displayTitle), - ...validateSkillFrontmatter(data.frontmatter), + ...validateSkillFrontmatter(frontmatter), ...validateAlwaysApply(data.alwaysApply), ]; /* Body-level `always-apply:` only needs to be well-formed when a @@ -1047,7 +1168,7 @@ export function createSkillMethods( if ( bodyAlwaysApply?.status === 'invalid' && typeof data.alwaysApply !== 'boolean' && - getAlwaysApplyFrontmatterValue(data.frontmatter) === undefined + getAlwaysApplyFrontmatterValue(frontmatter) === undefined ) { issues.push({ field: 'body.frontmatter.alwaysApply', @@ -1083,13 +1204,13 @@ export function createSkillMethods( throw error; } - const derived = deriveStructuredFrontmatterFields(data.frontmatter); + const derived = deriveStructuredFrontmatterFields(frontmatter); const doc = await Skill.create({ name: data.name, displayTitle: data.displayTitle, description: data.description, body: data.body ?? '', - frontmatter: data.frontmatter ?? {}, + frontmatter: frontmatter ?? {}, category: data.category ?? '', author: data.author, authorName: data.authorName, @@ -1099,7 +1220,7 @@ export function createSkillMethods( fileCount: 0, alwaysApply: resolveAlwaysApplyFromInput( data.alwaysApply, - data.frontmatter, + frontmatter, data.body, false, bodyAlwaysApply, @@ -1345,6 +1466,13 @@ export function createSkillMethods( if (!isValidObjectIdString(id)) { return { status: 'not_found' }; } + const normalizedFrontmatter = isPlainObject(update.frontmatter) + ? normalizeSkillFrontmatterKeys(update.frontmatter) + : undefined; + const frontmatter = + normalizedFrontmatter && 'frontmatter' in normalizedFrontmatter + ? normalizedFrontmatter.frontmatter + : update.frontmatter; /* Parse body's always-apply status once — reused for validation (precedence-aware, below) and the derivation cascade further @@ -1359,8 +1487,7 @@ export function createSkillMethods( if (update.body !== undefined) issues.push(...validateSkillBody(update.body)); if (update.displayTitle !== undefined) issues.push(...validateSkillDisplayTitle(update.displayTitle)); - if (update.frontmatter !== undefined) - issues.push(...validateSkillFrontmatter(update.frontmatter)); + if (update.frontmatter !== undefined) issues.push(...validateSkillFrontmatter(frontmatter)); if (update.alwaysApply !== undefined) issues.push(...validateAlwaysApply(update.alwaysApply)); /* Body-level `always-apply:` only needs to be well-formed when a higher-precedence source won't override it (see @@ -1371,7 +1498,7 @@ export function createSkillMethods( if ( bodyAlwaysApply?.status === 'invalid' && update.alwaysApply === undefined && - getAlwaysApplyFrontmatterValue(update.frontmatter) === undefined + getAlwaysApplyFrontmatterValue(frontmatter) === undefined ) { issues.push({ field: 'body.frontmatter.alwaysApply', @@ -1398,14 +1525,14 @@ export function createSkillMethods( if (update.source !== undefined) setPayload.source = update.source; if (update.sourceMetadata !== undefined) setPayload.sourceMetadata = update.sourceMetadata; if (update.frontmatter !== undefined) { - setPayload.frontmatter = update.frontmatter; + setPayload.frontmatter = frontmatter; /** * Derived columns track frontmatter — when frontmatter changes, the * derived view must follow. Fields the new frontmatter omits are * unset (back to schema default) so removing `disable-model-invocation` * from a SKILL.md re-enables model invocation on the next save. */ - const derived = deriveStructuredFrontmatterFields(update.frontmatter); + const derived = deriveStructuredFrontmatterFields(frontmatter); for (const key of ['disableModelInvocation', 'userInvocable', 'allowedTools'] as const) { if (derived[key] !== undefined) { setPayload[key] = derived[key]; @@ -1445,7 +1572,7 @@ export function createSkillMethods( derivedAlwaysApply = update.alwaysApply; } if (derivedAlwaysApply === undefined && update.frontmatter !== undefined) { - const fromFrontmatter = getAlwaysApplyFrontmatterValue(update.frontmatter); + const fromFrontmatter = getAlwaysApplyFrontmatterValue(frontmatter); if (typeof fromFrontmatter === 'boolean') { derivedAlwaysApply = fromFrontmatter; } diff --git a/packages/data-schemas/src/methods/skillSync.spec.ts b/packages/data-schemas/src/methods/skillSync.spec.ts index d384db1d7c..82c1b53726 100644 --- a/packages/data-schemas/src/methods/skillSync.spec.ts +++ b/packages/data-schemas/src/methods/skillSync.spec.ts @@ -132,6 +132,77 @@ describe('createSkillSyncMethods', () => { expect(success.errorMessage).toBeUndefined(); }); + it('persists the skipped skills of a partial run and treats it as a success timestamp', async () => { + const partial = await methods.upsertSkillSyncStatus({ + provider: 'github', + sourceId: 'librechat-skills', + status: 'partial', + finishedAt: new Date('2026-01-01T00:00:00.000Z'), + syncedSkillCount: 11, + skippedSkillCount: 2, + skippedSkills: [ + { + path: 'skills/broken', + name: 'broken', + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: 'skills/broken/SKILL.md: malformed frontmatter', + }, + ], + }); + + expect(partial).toMatchObject({ + status: 'partial', + syncedSkillCount: 11, + skippedSkillCount: 2, + lastSuccessAt: new Date('2026-01-01T00:00:00.000Z'), + }); + expect(partial.skippedSkills).toEqual([ + { + path: 'skills/broken', + name: 'broken', + errorCode: 'SKILL_PARSE_FAILED', + errorMessage: 'skills/broken/SKILL.md: malformed frontmatter', + }, + ]); + + const clean = await methods.upsertSkillSyncStatus({ + provider: 'github', + sourceId: 'librechat-skills', + status: 'succeeded', + syncedSkillCount: 13, + }); + + expect(clean.status).toBe('succeeded'); + expect(clean.skippedSkillCount).toBe(0); + expect(clean.skippedSkills).toEqual([]); + }); + + it('persists a skipped skill that lives at the repository root', async () => { + /* A root-level SKILL.md is discovered with an empty path, so a required + non-empty string here would reject the whole status document and lose + the partial result along with every skip reason in it. */ + const partial = await methods.upsertSkillSyncStatus({ + provider: 'github', + sourceId: 'librechat-skills', + status: 'partial', + syncedSkillCount: 1, + skippedSkillCount: 1, + skippedSkills: [ + { + path: '', + name: 'root-skill', + errorCode: 'DUPLICATE_SKILL_NAME', + errorMessage: 'GitHub source "librechat-skills" contains multiple skills named "root"', + }, + ], + }); + + expect(partial.status).toBe('partial'); + expect(partial.skippedSkills).toEqual([ + expect.objectContaining({ path: '', errorCode: 'DUPLICATE_SKILL_NAME' }), + ]); + }); + it('keeps status rows separate for the same source id in different tenants', async () => { await methods.upsertSkillSyncStatus({ provider: 'github', diff --git a/packages/data-schemas/src/methods/skillSync.ts b/packages/data-schemas/src/methods/skillSync.ts index 87c749f400..8d46d9aeb0 100644 --- a/packages/data-schemas/src/methods/skillSync.ts +++ b/packages/data-schemas/src/methods/skillSync.ts @@ -4,6 +4,7 @@ import type { ISkillSyncStatus, SkillSyncProvider, SkillSyncRunStatus, + ISkillSyncSkippedSkill, ISkillSyncStatusDocument, ISkillSyncCredential, ISkillSyncCredentialDocument, @@ -46,6 +47,8 @@ export type SkillSyncStatusInput = { syncedFileCount?: number; deletedSkillCount?: number; deletedFileCount?: number; + skippedSkillCount?: number; + skippedSkills?: ISkillSyncSkippedSkill[]; }; export type SkillSyncLockInput = { @@ -216,7 +219,9 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')): Ski async function upsertSkillSyncStatus(input: SkillSyncStatusInput): Promise { const Status = mongoose.models.SkillSyncStatus as Model; const now = new Date(); - const success = input.status === 'succeeded'; + /* A partial run published skills, so it advances `lastSuccessAt` the same + way a clean run does; the dropped skills live in `skippedSkills`. */ + const success = input.status === 'succeeded' || input.status === 'partial'; const failure = input.status === 'failed'; const setPayload: Partial = { status: input.status, @@ -231,6 +236,8 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')): Ski syncedFileCount: input.syncedFileCount ?? 0, deletedSkillCount: input.deletedSkillCount ?? 0, deletedFileCount: input.deletedFileCount ?? 0, + skippedSkillCount: input.skippedSkillCount ?? 0, + skippedSkills: input.skippedSkills ?? [], ...(success ? { lastSuccessAt: input.finishedAt ?? now } : {}), ...(failure ? { lastFailureAt: input.finishedAt ?? now } : {}), }; diff --git a/packages/data-schemas/src/schema/skill.ts b/packages/data-schemas/src/schema/skill.ts index 9ece6d82cc..35a1d779c5 100644 --- a/packages/data-schemas/src/schema/skill.ts +++ b/packages/data-schemas/src/schema/skill.ts @@ -108,9 +108,8 @@ const skillSchema: Schema = new Schema( }, /** * Structured YAML frontmatter bag (everything except `name`/`description`, - * which live as first-class columns). Validated in strict mode against - * `validateSkillFrontmatter` before write — unknown keys are rejected - * so any expansion of the allowed set is an explicit code change. + * which live as first-class columns). `validateSkillFrontmatter` type-checks + * recognized keys and bounds tolerated extension values before write. */ frontmatter: { type: Schema.Types.Mixed, diff --git a/packages/data-schemas/src/schema/skillSyncStatus.spec.ts b/packages/data-schemas/src/schema/skillSyncStatus.spec.ts new file mode 100644 index 0000000000..5165ab3299 --- /dev/null +++ b/packages/data-schemas/src/schema/skillSyncStatus.spec.ts @@ -0,0 +1,30 @@ +import mongoose from 'mongoose'; +import skillSyncStatusSchema from './skillSyncStatus'; + +const SkillSyncStatus = mongoose.model('SkillSyncStatusSchemaTest', skillSyncStatusSchema); + +describe('skillSyncStatusSchema', () => { + it('accepts an empty path for a skipped repository-root skill', () => { + const status = new SkillSyncStatus({ + provider: 'github', + sourceId: 'root-skills', + status: 'failed', + skippedSkillCount: 1, + skippedSkills: [{ path: '', errorCode: 'SKILL_PARSE_FAILED', errorMessage: 'Invalid YAML' }], + }); + + expect(status.validateSync()).toBeUndefined(); + }); + + it('still rejects a skipped skill without a path', () => { + const status = new SkillSyncStatus({ + provider: 'github', + sourceId: 'root-skills', + status: 'failed', + skippedSkillCount: 1, + skippedSkills: [{ errorCode: 'SKILL_PARSE_FAILED', errorMessage: 'Invalid YAML' }], + }); + + expect(status.validateSync()?.errors['skippedSkills.0.path']?.message).toBe('Path is required'); + }); +}); diff --git a/packages/data-schemas/src/schema/skillSyncStatus.ts b/packages/data-schemas/src/schema/skillSyncStatus.ts index 0413f9d02e..3fc6bebffb 100644 --- a/packages/data-schemas/src/schema/skillSyncStatus.ts +++ b/packages/data-schemas/src/schema/skillSyncStatus.ts @@ -1,5 +1,34 @@ import { Schema } from 'mongoose'; -import type { ISkillSyncStatusDocument } from '~/types/skillSync'; +import type { ISkillSyncSkippedSkill, ISkillSyncStatusDocument } from '~/types/skillSync'; + +const skippedSkillSchema = new Schema( + { + path: { + type: String, + default: null, + maxlength: 500, + validate: { + validator: (value: unknown) => typeof value === 'string', + message: 'Path is required', + }, + }, + name: { + type: String, + maxlength: 128, + }, + errorCode: { + type: String, + required: true, + maxlength: 64, + }, + errorMessage: { + type: String, + required: true, + maxlength: 500, + }, + }, + { _id: false }, +); const skillSyncStatusSchema: Schema = new Schema( { @@ -21,7 +50,7 @@ const skillSyncStatusSchema: Schema = new Schema( }, status: { type: String, - enum: ['idle', 'running', 'succeeded', 'failed', 'skipped'], + enum: ['idle', 'running', 'succeeded', 'partial', 'failed', 'skipped'], default: 'idle', required: true, }, @@ -79,6 +108,15 @@ const skillSyncStatusSchema: Schema = new Schema( default: 0, min: 0, }, + skippedSkillCount: { + type: Number, + default: 0, + min: 0, + }, + skippedSkills: { + type: [skippedSkillSchema], + default: undefined, + }, lockOwner: { type: String, }, diff --git a/packages/data-schemas/src/types/skillSync.ts b/packages/data-schemas/src/types/skillSync.ts index edd3bf5704..fa6a0783c5 100644 --- a/packages/data-schemas/src/types/skillSync.ts +++ b/packages/data-schemas/src/types/skillSync.ts @@ -1,7 +1,29 @@ import type { Document, Types } from 'mongoose'; export type SkillSyncProvider = 'github'; -export type SkillSyncRunStatus = 'idle' | 'running' | 'succeeded' | 'failed' | 'skipped'; +/** + * `partial` means the source published at least one skill while dropping + * others: a single unusable `SKILL.md` must not hide the skills that synced + * fine, and a run that quietly reported `succeeded` would hide the ones that + * did not. + */ +export type SkillSyncRunStatus = + | 'idle' + | 'running' + | 'succeeded' + | 'partial' + | 'failed' + | 'skipped'; + +/** One upstream skill a run could not publish, with the reason it was dropped. */ +export interface ISkillSyncSkippedSkill { + /** Repository path of the skill root that was skipped. */ + path: string; + /** Frontmatter name, when the failure happened late enough for one to exist. */ + name?: string; + errorCode: string; + errorMessage: string; +} export interface ISkillSyncCredential { provider: SkillSyncProvider; @@ -36,6 +58,9 @@ export interface ISkillSyncStatus { syncedFileCount: number; deletedSkillCount: number; deletedFileCount: number; + skippedSkillCount: number; + /** Capped sample of the skipped skills; `skippedSkillCount` is the full total. */ + skippedSkills?: ISkillSyncSkippedSkill[]; lockOwner?: string; lockExpiresAt?: Date; createdAt?: Date;