From cc72b2a7c79a41f5c9515264e68358fe9fedec48 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 24 May 2026 15:51:00 -0400 Subject: [PATCH] fix: Harden GitHub skill sync review paths --- .../api/src/skills/__tests__/import.test.ts | 9 ++ packages/api/src/skills/parse.ts | 13 ++- packages/api/src/skills/sync/github.spec.ts | 100 +++++++++++------- packages/api/src/skills/sync/github.ts | 6 ++ packages/api/src/skills/sync/scheduler.ts | 11 +- .../src/models/skillSyncCredential.ts | 2 + .../src/models/skillSyncStatus.ts | 2 + 7 files changed, 102 insertions(+), 41 deletions(-) diff --git a/packages/api/src/skills/__tests__/import.test.ts b/packages/api/src/skills/__tests__/import.test.ts index 626edbdb80..d577ee1556 100644 --- a/packages/api/src/skills/__tests__/import.test.ts +++ b/packages/api/src/skills/__tests__/import.test.ts @@ -173,6 +173,15 @@ describe('parseFrontmatter', () => { }); }); + it('returns empty fields when frontmatter YAML is malformed', () => { + const raw = `---\nname: [\n---\n\nbody`; + expect(parseFrontmatter(raw)).toEqual({ + name: '', + description: '', + invalidBooleans: [], + }); + }); + 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/parse.ts b/packages/api/src/skills/parse.ts index 01842bb950..2332d518b1 100644 --- a/packages/api/src/skills/parse.ts +++ b/packages/api/src/skills/parse.ts @@ -6,6 +6,7 @@ export type ParsedSkillMarkdown = { alwaysApply?: boolean; frontmatter?: Record; invalidBooleans: string[]; + parseError?: string; }; function isPlainObject(value: unknown): value is Record { @@ -58,7 +59,17 @@ export function parseSkillMarkdown(raw: string): ParsedSkillMarkdown { if (!block) { return { name: '', description: '', invalidBooleans: [] }; } - const parsed = yaml.load(block); + let parsed: unknown; + try { + parsed = yaml.load(block); + } catch (error) { + return { + name: '', + description: '', + invalidBooleans: [], + parseError: error instanceof Error ? error.message : 'Invalid YAML frontmatter', + }; + } const frontmatter = isPlainObject(parsed) ? normalizeFrontmatterKeys(parsed) : {}; const nameValue = getCaseInsensitive(frontmatter, 'name'); const descriptionValue = getCaseInsensitive(frontmatter, 'description'); diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 2b0074231c..a583310194 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -30,6 +30,47 @@ function blob(content: string) { }; } +function githubFetch( + skillMarkdown = '---\nname: research\ndescription: Research things\nalways-apply: true\n---\nBody', +): typeof fetch { + return jest.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes('/commits/main')) { + 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/research/SKILL.md', + mode: '100644', + type: 'blob', + sha: 'skill-md-sha', + url: 'https://api.github.test/blob/skill', + }, + { + path: 'skills/research/scripts/run.sh', + mode: '100644', + type: 'blob', + sha: 'file-sha', + size: 7, + url: 'https://api.github.test/blob/file', + }, + ], + }); + } + if (url.includes('/git/blobs/skill-md-sha')) { + return response(blob(skillMarkdown)); + } + if (url.includes('/git/blobs/file-sha')) { + return response(blob('echo ok')); + } + return response({ message: 'not found' }, 404); + }) as unknown as typeof fetch; +} + function makeSkill(input: CreateSkillInput): ISkill & { _id: Types.ObjectId } { return { _id: new Types.ObjectId(), @@ -135,44 +176,7 @@ function createDeps( saveBuffer: jest.fn(async () => ({ filepath: '/uploads/file-id__run.sh', source: 'local' })), deleteFile: jest.fn(async () => undefined), grantPermission: jest.fn(async () => undefined), - fetchFn: jest.fn(async (input: RequestInfo | URL) => { - const url = input.toString(); - if (url.includes('/commits/main')) { - 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/research/SKILL.md', - mode: '100644', - type: 'blob', - sha: 'skill-md-sha', - url: 'https://api.github.test/blob/skill', - }, - { - path: 'skills/research/scripts/run.sh', - mode: '100644', - type: 'blob', - sha: 'file-sha', - size: 7, - url: 'https://api.github.test/blob/file', - }, - ], - }); - } - if (url.includes('/git/blobs/skill-md-sha')) { - return response( - blob('---\nname: research\ndescription: Research things\nalways-apply: true\n---\nBody'), - ); - } - if (url.includes('/git/blobs/file-sha')) { - return response(blob('echo ok')); - } - return response({ message: 'not found' }, 404); - }) as unknown as typeof fetch, + fetchFn: githubFetch(), ...overrides, }; return deps; @@ -234,4 +238,24 @@ describe('createGitHubSkillSyncRunner', () => { }), ); }); + + it('marks a source failed and skips mirror deletion when SKILL.md frontmatter is malformed', async () => { + const deps = createDeps({ + fetchFn: githubFetch('---\nname: [\n---\nBody'), + }); + 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'), + }), + ); + }); }); diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index a6866bbb46..6f3f468dd3 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -485,6 +485,12 @@ async function upsertRemoteSkill(params: { }): Promise { const { deps, source, discovered, skillMdContent, commitSha, syncedAt } = params; const parsed = parseSkillMarkdown(skillMdContent); + if (parsed.parseError) { + throw new SkillSyncError( + 'SKILL_PARSE_FAILED', + `${discovered.rootPath}/SKILL.md contains invalid YAML frontmatter: ${parsed.parseError}`, + ); + } if (parsed.invalidBooleans.length > 0) { throw new SkillSyncError( 'SKILL_PARSE_FAILED', diff --git a/packages/api/src/skills/sync/scheduler.ts b/packages/api/src/skills/sync/scheduler.ts index ddde800825..aaa7ddde00 100644 --- a/packages/api/src/skills/sync/scheduler.ts +++ b/packages/api/src/skills/sync/scheduler.ts @@ -12,7 +12,14 @@ export function startGitHubSkillSyncScheduler(params: { runner: GitHubSkillSyncRunner; }): GitHubSkillSyncScheduler | null { const github = params.getConfig()?.github; - if (!github?.enabled || github.sources.length === 0) { + const sources = Array.isArray(github?.sources) ? github.sources : []; + const intervalMinutes = + typeof github?.intervalMinutes === 'number' && + Number.isFinite(github.intervalMinutes) && + github.intervalMinutes >= 5 + ? github.intervalMinutes + : 60; + if (!github?.enabled || sources.length === 0) { return null; } let stopped = false; @@ -31,7 +38,7 @@ export function startGitHubSkillSyncScheduler(params: { run(); } - timer = setInterval(run, github.intervalMinutes * 60 * 1000); + timer = setInterval(run, intervalMinutes * 60 * 1000); const scheduler = { stop: () => { diff --git a/packages/data-schemas/src/models/skillSyncCredential.ts b/packages/data-schemas/src/models/skillSyncCredential.ts index 2a1172fea5..537fc7c541 100644 --- a/packages/data-schemas/src/models/skillSyncCredential.ts +++ b/packages/data-schemas/src/models/skillSyncCredential.ts @@ -2,6 +2,8 @@ import skillSyncCredentialSchema from '~/schema/skillSyncCredential'; import type { ISkillSyncCredentialDocument } from '~/types/skillSync'; export function createSkillSyncCredentialModel(mongoose: typeof import('mongoose')) { + // GitHub skill sync is intentionally app-wide in v1; credentials are referenced by + // admin-managed config keys and are never returned by tenant-scoped APIs. return ( mongoose.models.SkillSyncCredential || mongoose.model('SkillSyncCredential', skillSyncCredentialSchema) diff --git a/packages/data-schemas/src/models/skillSyncStatus.ts b/packages/data-schemas/src/models/skillSyncStatus.ts index 61b4cae38f..64d0dc3cc2 100644 --- a/packages/data-schemas/src/models/skillSyncStatus.ts +++ b/packages/data-schemas/src/models/skillSyncStatus.ts @@ -2,6 +2,8 @@ import skillSyncStatusSchema from '~/schema/skillSyncStatus'; import type { ISkillSyncStatusDocument } from '~/types/skillSync'; export function createSkillSyncStatusModel(mongoose: typeof import('mongoose')) { + // GitHub skill sync status is intentionally app-wide in v1, matching the + // shared synced skills and the single scheduler lock across app processes. return ( mongoose.models.SkillSyncStatus || mongoose.model('SkillSyncStatus', skillSyncStatusSchema)