diff --git a/packages/api/src/skills/__tests__/import.integration.spec.ts b/packages/api/src/skills/__tests__/import.integration.spec.ts new file mode 100644 index 0000000000..a51435a50a --- /dev/null +++ b/packages/api/src/skills/__tests__/import.integration.spec.ts @@ -0,0 +1,195 @@ +import JSZip from 'jszip'; +import mongoose, { Types } from 'mongoose'; +import { MongoMemoryServer } from 'mongodb-memory-server'; +import { logger, createModels, createMethods } from '@librechat/data-schemas'; +import type { AllMethods, ISkill } from '@librechat/data-schemas'; +import type { Response } from 'express'; +import type { ImportSkillDeps } from '../import'; +import { createImportHandler } from '../import'; + +logger.silent = true; + +/** + * End-to-end coverage for #14208: the import handler wired to the REAL + * `createSkill`, asserting the persisted document rather than the arguments a + * mock received. `import.test.ts` pins the frontmatter bag the handler builds; + * this pins the columns that bag actually produces, so the two halves of the + * fix can't drift apart silently. + */ + +type ImportRequest = Parameters>[0]; +type CapturedResponse = Response & { statusCode?: number; body?: unknown }; + +let mongoServer: MongoMemoryServer; +let methods: AllMethods; +let author: Types.ObjectId; + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + createModels(mongoose); + methods = createMethods(mongoose); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +beforeEach(async () => { + author = new Types.ObjectId(); + await mongoose.connection.collection('skills').deleteMany({}); +}); + +function captureResponse(): CapturedResponse { + const res = {} as CapturedResponse; + res.status = jest.fn((statusCode: number) => { + res.statusCode = statusCode; + return res; + }) as CapturedResponse['status']; + res.json = jest.fn((body: unknown) => { + res.body = body; + return res; + }) as CapturedResponse['json']; + return res; +} + +function importDeps(): ImportSkillDeps { + return { + createSkill: methods.createSkill, + getSkillById: methods.getSkillById, + deleteSkill: methods.deleteSkill, + upsertSkillFile: methods.upsertSkillFile, + saveBuffer: jest.fn(async () => ({ filepath: '/tmp/skill-file', source: 'local' })), + grantPermission: jest.fn(async () => undefined), + }; +} + +function request(content: string | Buffer, originalname: string): ImportRequest { + return { + user: { id: author.toString(), _id: author, username: 'importer' }, + file: { + originalname, + buffer: typeof content === 'string' ? Buffer.from(content) : content, + }, + } as unknown as ImportRequest; +} + +async function zipped(skillMarkdown: string): Promise { + const zip = new JSZip(); + zip.file('SKILL.md', skillMarkdown); + return zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' }); +} + +async function persisted(res: CapturedResponse): Promise { + const created = res.body as ISkill & { _id: Types.ObjectId }; + const reloaded = await methods.getSkillById(created._id); + if (!reloaded) { + throw new Error('Imported skill was not persisted'); + } + return reloaded; +} + +/** Verbatim from the issue's reproduction steps. */ +const REPORTED_SKILL_MD = [ + '---', + 'name: test-skill', + 'description: test', + 'always-apply: true', + 'user-invocable: false', + 'disable-model-invocation: true', + '---', + 'Test body.', +].join('\n'); + +describe('POST /api/skills/import — invocation-mode frontmatter (#14208)', () => { + it('persists all three invocation-mode flags from a markdown upload', async () => { + const res = captureResponse(); + await createImportHandler(importDeps())(request(REPORTED_SKILL_MD, 'test-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + const skill = await persisted(res); + expect(skill.name).toBe('test-skill'); + expect(skill.alwaysApply).toBe(true); + expect(skill.userInvocable).toBe(false); + expect(skill.disableModelInvocation).toBe(true); + }); + + it('persists all three invocation-mode flags from an archive upload', async () => { + const res = captureResponse(); + await createImportHandler(importDeps())( + request(await zipped(REPORTED_SKILL_MD), 'test-skill.skill'), + res, + ); + + expect(res.status).toHaveBeenCalledWith(201); + const skill = await persisted(res); + expect(skill.alwaysApply).toBe(true); + expect(skill.userInvocable).toBe(false); + expect(skill.disableModelInvocation).toBe(true); + }); + + it('leaves the schema defaults in place when the file declares no flags', async () => { + const res = captureResponse(); + const markdown = '---\nname: plain-skill\ndescription: A skill with no flags.\n---\n\nbody'; + await createImportHandler(importDeps())(request(markdown, 'plain-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + const skill = await persisted(res); + expect(skill.alwaysApply).toBe(false); + expect(skill.userInvocable).toBe(true); + expect(skill.disableModelInvocation).toBe(false); + expect(skill.allowedTools).toBeUndefined(); + }); + + it('persists the allowedTools column declared by the uploaded file', async () => { + const res = captureResponse(); + const markdown = [ + '---', + 'name: tooled-skill', + 'description: A skill declaring extra tools.', + 'allowed-tools:', + ' - web_search', + ' - file_search', + '---', + 'body', + ].join('\n'); + await createImportHandler(importDeps())(request(markdown, 'tooled-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + const skill = await persisted(res); + expect(skill.allowedTools).toEqual(['web_search', 'file_search']); + }); + + it('imports a file whose extra frontmatter would fail strict validation', async () => { + const res = captureResponse(); + const markdown = [ + '---', + 'name: ecosystem-skill', + 'description: Authored for another skill ecosystem.', + 'icon: rocket', + 'version: 1.0', + 'user-invocable: false', + '---', + 'body', + ].join('\n'); + await createImportHandler(importDeps())(request(markdown, 'ecosystem-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + const skill = await persisted(res); + expect(skill.userInvocable).toBe(false); + expect(skill.frontmatter).toEqual({ 'user-invocable': false }); + }); + + it('rejects a malformed flag value instead of persisting a skill at the default', async () => { + const res = captureResponse(); + const markdown = + '---\nname: broken-skill\ndescription: A skill with a bad flag.\nuser-invocable: yes\n---\n\nbody'; + await createImportHandler(importDeps())(request(markdown, 'broken-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(400); + await expect( + mongoose.connection.collection('skills').countDocuments({ name: 'broken-skill' }), + ).resolves.toBe(0); + }); +}); diff --git a/packages/api/src/skills/__tests__/import.test.ts b/packages/api/src/skills/__tests__/import.test.ts index 55416967d2..c8e9e3f1d7 100644 --- a/packages/api/src/skills/__tests__/import.test.ts +++ b/packages/api/src/skills/__tests__/import.test.ts @@ -110,6 +110,18 @@ async function zipWithSkillMarkdown(skillMarkdown: string): Promise { return zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' }); } +/** The reporter's repro from #14208: all three invocation-mode flags off-default. */ +const INVOCATION_MODE_SKILL_MD = [ + '---', + 'name: test-skill', + 'description: A skill that restricts its invocation channels.', + 'always-apply: true', + 'user-invocable: false', + 'disable-model-invocation: true', + '---', + 'Test body.', +].join('\n'); + describe('parseFrontmatter', () => { it('extracts name + description from a minimal frontmatter block', () => { const raw = `---\nname: demo\ndescription: A demo skill.\n---\n\n# Body`; @@ -117,6 +129,7 @@ describe('parseFrontmatter', () => { name: 'demo', description: 'A demo skill.', alwaysApply: undefined, + frontmatter: {}, invalidBooleans: [], }); }); @@ -127,6 +140,7 @@ describe('parseFrontmatter', () => { name: '123', description: '2024', alwaysApply: undefined, + frontmatter: {}, invalidBooleans: [], }); }); @@ -137,6 +151,7 @@ describe('parseFrontmatter', () => { name: 'legal', description: 'Legal rules.', alwaysApply: true, + frontmatter: { 'always-apply': true }, invalidBooleans: [], }); }); @@ -147,16 +162,18 @@ describe('parseFrontmatter', () => { name: 'optional', description: 'Optional rules.', alwaysApply: false, + frontmatter: { 'always-apply': false }, invalidBooleans: [], }); }); - it('extracts alwaysApply: true', () => { + it('extracts alwaysApply: true and canonicalizes the alias in the bag', () => { const raw = `---\nname: legal\ndescription: Legal rules.\nalwaysApply: true\n---\n\n# Legal body`; expect(parseFrontmatter(raw)).toEqual({ name: 'legal', description: 'Legal rules.', alwaysApply: true, + frontmatter: { 'always-apply': true }, invalidBooleans: [], }); }); @@ -233,6 +250,7 @@ describe('parseFrontmatter', () => { name: 'quoted-name', description: 'quoted desc', alwaysApply: true, + frontmatter: { 'always-apply': true }, invalidBooleans: [], }); }); @@ -242,6 +260,7 @@ describe('parseFrontmatter', () => { expect(parseFrontmatter(raw)).toEqual({ name: '', description: '', + frontmatter: {}, invalidBooleans: [], }); }); @@ -252,6 +271,7 @@ describe('parseFrontmatter', () => { name: 'prologue', description: 'Has leading whitespace.', alwaysApply: undefined, + frontmatter: {}, invalidBooleans: [], }); }); @@ -262,6 +282,7 @@ describe('parseFrontmatter', () => { name: 'marker', description: 'first ---not a closing fence last', alwaysApply: false, + frontmatter: { 'always-apply': false }, invalidBooleans: [], }); }); @@ -271,6 +292,7 @@ describe('parseFrontmatter', () => { expect(parseFrontmatter(raw)).toEqual({ name: '', description: '', + frontmatter: {}, invalidBooleans: [], }); }); @@ -328,6 +350,212 @@ describe('parseFrontmatter', () => { expect(result.alwaysApply).toBe(false); expect(result.invalidBooleans).toEqual([]); }); + + it('extracts user-invocable and disable-model-invocation alongside always-apply', () => { + const raw = [ + '---', + 'name: test-skill', + 'description: test', + 'always-apply: true', + 'user-invocable: false', + 'disable-model-invocation: true', + '---', + 'Test body.', + ].join('\n'); + + expect(parseFrontmatter(raw)).toEqual({ + name: 'test-skill', + description: 'test', + alwaysApply: true, + userInvocable: false, + disableModelInvocation: true, + frontmatter: { + 'always-apply': true, + 'user-invocable': false, + 'disable-model-invocation': true, + }, + invalidBooleans: [], + }); + }); + + it.each([ + ['user-invocable', 'userInvocable'], + ['disable-model-invocation', 'disableModelInvocation'], + ] as const)('extracts %s: false', (key, field) => { + const raw = `---\nname: n\ndescription: d\n${key}: false\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result[field]).toBe(false); + expect(result.frontmatter).toEqual({ [key]: false }); + expect(result.invalidBooleans).toEqual([]); + }); + + it.each(['user-invocable', 'disable-model-invocation'])( + 'flags a non-boolean %s value instead of silently defaulting it', + (key) => { + const raw = `---\nname: n\ndescription: d\n${key}: yes\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.invalidBooleans).toEqual([key]); + expect(result.frontmatter).toEqual({}); + }, + ); + + it.each(['user-invocable', 'disable-model-invocation'])( + 'treats an empty %s value as absent (mid-edit placeholder)', + (key) => { + const raw = `---\nname: n\ndescription: d\n${key}:\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.invalidBooleans).toEqual([]); + expect(result.frontmatter).toEqual({}); + }, + ); + + it('normalizes quoted and comment-trailed invocation booleans', () => { + const raw = [ + '---', + 'name: n', + 'description: d', + 'user-invocable: "false" # manual off', + "disable-model-invocation: 'true'", + '---', + 'body', + ].join('\n'); + const result = parseFrontmatter(raw); + + expect(result.userInvocable).toBe(false); + expect(result.disableModelInvocation).toBe(true); + expect(result.frontmatter).toEqual({ + 'user-invocable': false, + 'disable-model-invocation': true, + }); + }); + + it('is case-insensitive on the new flag keys', () => { + const raw = `---\nname: n\ndescription: d\nUser-Invocable: FALSE\nDISABLE-MODEL-INVOCATION: True\n---\n\nbody`; + const result = parseFrontmatter(raw); + + expect(result.userInvocable).toBe(false); + expect(result.disableModelInvocation).toBe(true); + }); + + it('reports every malformed flag at once', () => { + const raw = [ + '---', + 'name: n', + 'description: d', + 'always-apply: tru', + 'user-invocable: nope', + 'disable-model-invocation: 1', + '---', + 'body', + ].join('\n'); + + expect(parseFrontmatter(raw).invalidBooleans).toEqual([ + 'always-apply', + 'user-invocable', + 'disable-model-invocation', + ]); + }); + + it('keeps recognized non-flag frontmatter in the bag', () => { + const raw = [ + '---', + 'name: n', + 'description: d', + 'when-to-use: When demoing.', + 'allowed-tools:', + ' - web_search', + 'license: MIT', + '---', + 'body', + ].join('\n'); + + expect(parseFrontmatter(raw).frontmatter).toEqual({ + 'when-to-use': 'When demoing.', + 'allowed-tools': ['web_search'], + license: 'MIT', + }); + }); + + it('drops unrecognized and malformed non-flag keys rather than failing the parse', () => { + const raw = [ + '---', + 'name: n', + 'description: d', + 'icon: rocket', + 'version: 1.0', + 'user-invocable: false', + '---', + 'body', + ].join('\n'); + const result = parseFrontmatter(raw); + + expect(result.frontmatter).toEqual({ 'user-invocable': false }); + expect(result.invalidBooleans).toEqual([]); + expect(result.parseError).toBeUndefined(); + }); + + it.each([ + ['always-apply', 'true', 'alwaysApply', true], + ['user-invocable', 'false', 'userInvocable', false], + ['disable-model-invocation', 'true', 'disableModelInvocation', true], + ] as const)( + 'resolves %s when YAML continues the value on the next line', + (key, text, field, expected) => { + const raw = `---\nname: n\ndescription: d\n${key}:\n ${text}\n---\n\nbody`; + const result = parseFrontmatter(raw); + + expect(result[field]).toBe(expected); + expect(result.invalidBooleans).toEqual([]); + expect(result.frontmatter).toEqual({ [key]: expected }); + }, + ); + + it('resolves flags when the whole frontmatter mapping is indented', () => { + const raw = `---\n name: n\n description: d\n user-invocable: false\n---\n\nbody`; + const result = parseFrontmatter(raw); + + expect(result.userInvocable).toBe(false); + expect(result.invalidBooleans).toEqual([]); + }); + + it('ignores a nested mapping key that reuses a flag name', () => { + const raw = [ + '---', + 'name: n', + 'description: d', + 'metadata:', + ' user-invocable: nonsense', + 'user-invocable: false', + '---', + 'body', + ].join('\n'); + const result = parseFrontmatter(raw); + + expect(result.userInvocable).toBe(false); + expect(result.invalidBooleans).toEqual([]); + }); + + it('does not read a flag from a nested mapping when the top level omits it', () => { + const raw = `---\nname: n\ndescription: d\nmetadata:\n user-invocable: nonsense\n---\n\nbody`; + const result = parseFrontmatter(raw); + + expect(result.userInvocable).toBeUndefined(); + expect(result.invalidBooleans).toEqual([]); + }); + + it('preserves frontmatter key order when rewriting flags', () => { + const raw = [ + '---', + 'name: n', + 'description: d', + 'user-invocable: "false"', + 'license: MIT', + '---', + 'body', + ].join('\n'); + + expect(Object.keys(parseFrontmatter(raw).frontmatter)).toEqual(['user-invocable', 'license']); + }); }); describe('createImportHandler', () => { @@ -415,4 +643,139 @@ describe('createImportHandler', () => { ); expect(deps.createSkill).not.toHaveBeenCalled(); }); + + it('forwards every invocation-mode flag from a markdown import', async () => { + const deps = mockImportDeps(); + const handler = createImportHandler(deps); + const res = mockResponse(); + + await handler(mockMarkdownRequest(INVOCATION_MODE_SKILL_MD, 'test-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'test-skill', + alwaysApply: true, + frontmatter: { + 'always-apply': true, + 'user-invocable': false, + 'disable-model-invocation': true, + }, + }), + ); + }); + + it('forwards every invocation-mode flag from an archive import', async () => { + const deps = mockImportDeps(); + const handler = createImportHandler(deps); + const res = mockResponse(); + const buffer = await zipWithSkillMarkdown(INVOCATION_MODE_SKILL_MD); + + await handler(mockZipRequest(buffer), res); + + expect(res.status).toHaveBeenCalledWith(201); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + alwaysApply: true, + frontmatter: { + 'always-apply': true, + 'user-invocable': false, + 'disable-model-invocation': true, + }, + }), + ); + }); + + it('forwards allowed-tools so the derived column is populated on import', async () => { + const deps = mockImportDeps(); + const handler = createImportHandler(deps); + const res = mockResponse(); + const markdown = [ + '---', + 'name: tooled-skill', + 'description: A skill that declares extra tools.', + 'allowed-tools:', + ' - web_search', + ' - file_search', + '---', + 'body', + ].join('\n'); + + await handler(mockMarkdownRequest(markdown, 'tooled-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + frontmatter: { 'allowed-tools': ['web_search', 'file_search'] }, + }), + ); + }); + + it.each(['user-invocable', 'disable-model-invocation'])( + 'rejects a malformed %s value instead of importing it at the schema default', + async (key) => { + const deps = mockImportDeps(); + const handler = createImportHandler(deps); + const res = mockResponse(); + const markdown = `---\nname: broken-skill\ndescription: A skill with a bad flag.\n${key}: yes\n---\n\nbody`; + + await handler(mockMarkdownRequest(markdown, 'broken-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.body).toEqual( + expect.objectContaining({ + error: 'Validation failed', + issues: [ + { + field: `frontmatter.${key}`, + code: 'INVALID_TYPE', + message: `"${key}" must be a boolean (true or false)`, + }, + ], + }), + ); + expect(deps.createSkill).not.toHaveBeenCalled(); + }, + ); + + it('imports a file carrying unknown frontmatter keys, dropping only those keys', async () => { + const deps = mockImportDeps(); + const handler = createImportHandler(deps); + const res = mockResponse(); + const markdown = [ + '---', + 'name: ecosystem-skill', + 'description: Authored for another skill ecosystem.', + 'icon: rocket', + 'version: 1.0', + 'user-invocable: false', + '---', + 'body', + ].join('\n'); + + await handler(mockMarkdownRequest(markdown, 'ecosystem-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + frontmatter: { 'user-invocable': false }, + }), + ); + }); + + it('sends an empty frontmatter bag when the file has no frontmatter block', async () => { + const deps = mockImportDeps(); + const handler = createImportHandler(deps); + const res = mockResponse(); + + await handler(mockMarkdownRequest('# Just a body', 'bodyless-skill.md'), res); + + expect(res.status).toHaveBeenCalledWith(201); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'bodyless-skill', + frontmatter: {}, + }), + ); + }); }); diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index 5e1224a1fb..420b7264a7 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -1,7 +1,7 @@ import path from 'path'; import JSZip from 'jszip'; import crypto from 'crypto'; -import { logger } from '@librechat/data-schemas'; +import { logger, pickValidFrontmatter } from '@librechat/data-schemas'; import { ResourceType, AccessRoleIds, PrincipalType } from 'librechat-data-provider'; import type { ISkill, @@ -13,73 +13,101 @@ import type { import type { Request, Response } from 'express'; import type { Types } from 'mongoose'; import type { ImportLimits } from './limits'; +import { parseSkillMarkdown, toCleanFrontmatter } from './parse'; import { resolveRequestTenantId } from '~/middleware/tenant'; import { DEFAULT_SKILL_IMPORT_LIMITS } from './limits'; import { isSafeSkillFilePath } from './path'; -import { parseSkillMarkdown } from './parse'; const SKILL_MD = 'SKILL.md'; export type { ImportLimits } from './limits'; -/** - * YAML frontmatter parser — extracts the first-class fields LibreChat - * persists as columns (`name`, `description`, `alwaysApply`) out of a - * SKILL.md file. Intentionally narrow: the full frontmatter validator in - * `packages/data-schemas/src/methods/skill.ts` covers the wire contract; - * this parser only needs to hand `createSkill` the columns it populates. - * - * When a known boolean field (currently `always-apply` plus the accepted - * `alwaysApply` alias) is present - * with a value that isn't recognizable as `true`/`false`, the parser - * records it on `invalidBooleans[]` so the import handler can surface - * a 400 instead of silently dropping the flag. Without this signal, - * authoring mistakes like `alwaysApply: yes` would be lossy-converted - * to "not always-applied" and the user would never learn their - * frontmatter was malformed. - * - * Exported for unit testing only — prefer `createImportHandler` at runtime. - */ -export function parseFrontmatter(raw: string): { +type ParsedSkillFile = { name: string; description: string; alwaysApply?: boolean; + userInvocable?: boolean; + disableModelInvocation?: boolean; + /** + * Frontmatter bag ready for `createSkill`: cleaned of the fields that live + * as their own columns, and narrowed to entries strict validation accepts. + */ + frontmatter: Record; /** Keys that carried non-boolean values for fields that must be boolean. */ invalidBooleans: string[]; parseError?: string; -} { +}; + +/** + * YAML frontmatter parser for an uploaded SKILL.md — hands `createSkill` the + * first-class columns (`name`, `description`, `alwaysApply`) plus the + * frontmatter bag the remaining columns are derived from. Every + * invocation-mode flag (`always-apply` with its accepted `alwaysApply` alias, + * `user-invocable`, `disable-model-invocation`) reaches the document through + * that bag, so an uploaded file governs its own invocation channels the same + * way an explicit `frontmatter` payload to `POST /api/skills` does. + * + * When a boolean field carries a value that isn't recognizable as + * `true`/`false`, the parser records it on `invalidBooleans[]` so the import + * handler can surface a 400 instead of silently dropping the flag. Without + * this signal, authoring mistakes like `user-invocable: yes` would be + * lossy-converted to the schema default and the user would never learn their + * frontmatter was malformed. + * + * Entries outside the flags are filtered rather than rejected: an uploaded + * file is not something the uploader typed into a form, so a stray + * `version: 1.0` or a bespoke key from another skill ecosystem must not fail + * the upload. Admin-authored paths (GitHub sync, deployment skills) keep the + * strict validator's hard failure instead. + * + * Exported for unit testing only — prefer `createImportHandler` at runtime. + */ +export function parseFrontmatter(raw: string): ParsedSkillFile { const parsed = parseSkillMarkdown(raw); - const result: { - name: string; - description: string; - alwaysApply?: boolean; - invalidBooleans: string[]; - parseError?: string; - } = { + const result: ParsedSkillFile = { name: parsed.name, description: parsed.description, + alwaysApply: parsed.alwaysApply, + userInvocable: parsed.userInvocable, + disableModelInvocation: parsed.disableModelInvocation, + frontmatter: pickValidFrontmatter(toCleanFrontmatter(parsed)), invalidBooleans: parsed.invalidBooleans, }; if (parsed.parseError) { result.parseError = parsed.parseError; } - if ('alwaysApply' in parsed) { - result.alwaysApply = parsed.alwaysApply; - } return result; } -function sendFrontmatterParseError(res: Response, parseError: string) { - return res.status(400).json({ - error: 'Validation failed', - issues: [ - { - field: 'frontmatter', - code: 'INVALID_YAML', - message: `Invalid YAML frontmatter: ${parseError}`, - }, - ], - }); +/** + * Reject a SKILL.md whose frontmatter can't be trusted — unparseable YAML, or + * a boolean invocation-mode flag with a value that is neither `true` nor + * `false`. Returns the sent response, or `null` when the parse is clean. + */ +function sendFrontmatterIssues(res: Response, parsed: ParsedSkillFile): Response | null { + if (parsed.parseError) { + return res.status(400).json({ + error: 'Validation failed', + issues: [ + { + field: 'frontmatter', + code: 'INVALID_YAML', + message: `Invalid YAML frontmatter: ${parsed.parseError}`, + }, + ], + }); + } + if (parsed.invalidBooleans.length > 0) { + return res.status(400).json({ + error: 'Validation failed', + issues: parsed.invalidBooleans.map((key) => ({ + field: `frontmatter.${key}`, + code: 'INVALID_TYPE', + message: `"${key}" must be a boolean (true or false)`, + })), + }); + } + return null; } /** Type guard for validation errors thrown by data-schemas. */ @@ -247,20 +275,12 @@ async function handleMarkdown( ) { const content = file.buffer.toString('utf-8'); - const { name, description, alwaysApply, invalidBooleans, parseError } = parseFrontmatter(content); - if (parseError) { - return sendFrontmatterParseError(res, parseError); - } - if (invalidBooleans.length > 0) { - return res.status(400).json({ - error: 'Validation failed', - issues: invalidBooleans.map((key) => ({ - field: `frontmatter.${key}`, - code: 'INVALID_TYPE', - message: `"${key}" must be a boolean (true or false)`, - })), - }); + const parsed = parseFrontmatter(content); + const frontmatterError = sendFrontmatterIssues(res, parsed); + if (frontmatterError) { + return frontmatterError; } + const { name, description, alwaysApply, frontmatter } = parsed; const inferredName = name || file.originalname @@ -280,6 +300,7 @@ async function handleMarkdown( name: inferredName, description: description || inferredName, body: content, + frontmatter, author: authorId, authorName, alwaysApply, @@ -361,21 +382,12 @@ async function handleZip( return res.status(400).json({ error: 'SKILL.md exceeds maximum file size' }); } - const { name, description, alwaysApply, invalidBooleans, parseError } = - parseFrontmatter(skillMdContent); - if (parseError) { - return sendFrontmatterParseError(res, parseError); - } - if (invalidBooleans.length > 0) { - return res.status(400).json({ - error: 'Validation failed', - issues: invalidBooleans.map((key) => ({ - field: `frontmatter.${key}`, - code: 'INVALID_TYPE', - message: `"${key}" must be a boolean (true or false)`, - })), - }); + const parsed = parseFrontmatter(skillMdContent); + const frontmatterError = sendFrontmatterIssues(res, parsed); + if (frontmatterError) { + return frontmatterError; } + const { name, description, alwaysApply, frontmatter } = parsed; const inferredName = name || file.originalname @@ -395,6 +407,7 @@ async function handleZip( name: inferredName, description: description || inferredName, body: skillMdContent, + frontmatter, author: authorId, authorName, alwaysApply, diff --git a/packages/api/src/skills/parse.ts b/packages/api/src/skills/parse.ts index 041e87ed5e..2b1b40580d 100644 --- a/packages/api/src/skills/parse.ts +++ b/packages/api/src/skills/parse.ts @@ -1,14 +1,24 @@ import yaml from 'js-yaml'; +import { SKILL_BOOLEAN_FLAGS } from '@librechat/data-schemas'; +import type { SkillBooleanFlag, SkillBooleanColumn } from '@librechat/data-schemas'; export type ParsedSkillMarkdown = { name: string; description: string; alwaysApply?: boolean; + userInvocable?: boolean; + disableModelInvocation?: boolean; frontmatter?: Record; invalidBooleans: string[]; parseError?: string; }; +const SKILL_BOOLEAN_FLAG_BY_KEY = new Map( + SKILL_BOOLEAN_FLAGS.flatMap((flag) => + [flag.key, ...flag.aliases].map((key) => [key, flag] as const), + ), +); + function isPlainObject(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } @@ -33,17 +43,29 @@ function extractFrontmatterBlock(raw: string): string | null { } function getCaseInsensitive(frontmatter: Record, key: string): unknown { - const entry = Object.entries(frontmatter).find(([candidate]) => candidate.toLowerCase() === key); + const target = key.toLowerCase(); + const entry = Object.entries(frontmatter).find( + ([candidate]) => candidate.toLowerCase() === target, + ); return entry?.[1]; } function hasCaseInsensitive(frontmatter: Record, key: string): boolean { - return Object.keys(frontmatter).some((candidate) => candidate.toLowerCase() === key); + const target = key.toLowerCase(); + return Object.keys(frontmatter).some((candidate) => candidate.toLowerCase() === target); } +/** + * Recover the text a key carried on its own line, used to cross-check the + * value the YAML parser resolved. Anchored at column zero so a nested mapping + * that reuses a flag name (`metadata:` → ` user-invocable: ...`) can't shadow + * the top-level key; nested keys never reach the top-level bag, so matching one + * here would attribute an unrelated value to the flag. Returns `undefined` when + * no line matches, which callers treat as "nothing to cross-check". + */ function getRawFrontmatterValue(block: string, key: string): string | undefined { const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const pattern = new RegExp(`^\\s*${escapedKey}\\s*:\\s*(.*)$`, 'i'); + const pattern = new RegExp(`^${escapedKey}\\s*:\\s*(.*)$`, 'i'); const line = block.split('\n').find((candidate) => pattern.test(candidate)); const match = line?.match(pattern); return match?.[1]; @@ -75,6 +97,15 @@ function normalizeFrontmatterKeys(frontmatter: Record): Record< function parseBoolean(value: unknown, rawValue?: string): boolean | undefined { const raw = rawValue === undefined ? undefined : stripInlineComment(rawValue).toLowerCase(); if (typeof value === 'boolean') { + /* The raw line exists only to reject inline text that contradicts the + resolved value (an explicit `!!bool` tag, say). When the key's line holds + no value of its own — because YAML continues it on the following line, or + the whole mapping is indented and the line scan finds nothing — there is + nothing to contradict, so trust the boolean the parser resolved. Treating + it as absent instead would silently discard a declared flag. */ + if (raw === undefined || raw.length === 0) { + return value; + } return raw === 'true' || raw === 'false' ? value : undefined; } if (typeof value !== 'string') { @@ -94,6 +125,43 @@ function hasBooleanPlaceholder(rawValue?: string): boolean { return rawValue !== undefined && stripInlineComment(rawValue).length === 0; } +type ResolvedBooleanFlag = { value?: boolean; invalidKey?: string }; + +/** + * Read a flag key already known to be present. A value that is neither + * `true` nor `false` is reported on `invalidKey` — under the key's authored + * spelling — rather than silently collapsing to "off", except for an empty + * value (`user-invocable:` with nothing after it), which is a mid-edit + * placeholder and treated as absent. + */ +function readPresentBooleanFlag( + frontmatter: Record, + block: string, + key: string, +): ResolvedBooleanFlag { + const rawValue = getRawFrontmatterValue(block, key); + const value = parseBoolean(getCaseInsensitive(frontmatter, key), rawValue); + if (value === undefined && !hasBooleanPlaceholder(rawValue)) { + return { invalidKey: key }; + } + return { value }; +} + +function resolveBooleanFlag( + frontmatter: Record, + block: string, + flag: SkillBooleanFlag, +): ResolvedBooleanFlag { + if (hasCaseInsensitive(frontmatter, flag.key)) { + return readPresentBooleanFlag(frontmatter, block, flag.key); + } + const alias = flag.aliases.find((candidate) => hasCaseInsensitive(frontmatter, candidate)); + if (alias !== undefined) { + return readPresentBooleanFlag(frontmatter, block, alias); + } + return {}; +} + function toScalarString(value: unknown): string { if (typeof value === 'string') { return value; @@ -124,12 +192,6 @@ export function parseSkillMarkdown(raw: string): ParsedSkillMarkdown { const nameValue = getCaseInsensitive(frontmatter, 'name'); const descriptionValue = getCaseInsensitive(frontmatter, 'description'); const whenToUseValue = getCaseInsensitive(frontmatter, 'when-to-use'); - const hasCanonicalAlwaysApply = hasCaseInsensitive(frontmatter, 'always-apply'); - const hasAliasAlwaysApply = hasCaseInsensitive(frontmatter, 'alwaysapply'); - const canonicalAlwaysApplyValue = getCaseInsensitive(frontmatter, 'always-apply'); - const aliasAlwaysApplyValue = getCaseInsensitive(frontmatter, 'alwaysapply'); - const rawCanonicalAlwaysApplyValue = getRawFrontmatterValue(block, 'always-apply'); - const rawAliasAlwaysApplyValue = getRawFrontmatterValue(block, 'alwaysApply'); const name = toScalarString(nameValue); let description = ''; if (descriptionValue !== undefined) { @@ -137,24 +199,60 @@ export function parseSkillMarkdown(raw: string): ParsedSkillMarkdown { } else if (whenToUseValue !== undefined) { description = toScalarString(whenToUseValue); } - let alwaysApply: boolean | undefined; + const flagValues: Partial> = {}; const invalidBooleans: string[] = []; - if (hasCanonicalAlwaysApply) { - alwaysApply = parseBoolean(canonicalAlwaysApplyValue, rawCanonicalAlwaysApplyValue); - if (alwaysApply === undefined && !hasBooleanPlaceholder(rawCanonicalAlwaysApplyValue)) { - invalidBooleans.push('always-apply'); + for (const flag of SKILL_BOOLEAN_FLAGS) { + const { value, invalidKey } = resolveBooleanFlag(frontmatter, block, flag); + if (invalidKey !== undefined) { + invalidBooleans.push(invalidKey); + continue; } - } else if (hasAliasAlwaysApply) { - alwaysApply = parseBoolean(aliasAlwaysApplyValue, rawAliasAlwaysApplyValue); - if (alwaysApply === undefined && !hasBooleanPlaceholder(rawAliasAlwaysApplyValue)) { - invalidBooleans.push('alwaysApply'); + if (value !== undefined) { + flagValues[flag.column] = value; } } return { name, description, - alwaysApply, + alwaysApply: flagValues.alwaysApply, + userInvocable: flagValues.userInvocable, + disableModelInvocation: flagValues.disableModelInvocation, frontmatter, invalidBooleans, }; } + +/** + * Reduce a parsed frontmatter bag to what a skill document should persist: + * `name`/`description` live in their own columns, and every boolean flag is + * rewritten from the parser's resolved value under its canonical key. + * + * That rewrite is what keeps the bag honest. Values the parser could not read + * as booleans — a mid-edit `user-invocable:` placeholder, a + * `disable-model-invocation: yes` typo, a quoted `"true"` — never reach the + * document as-is, so the bag both passes strict frontmatter validation and + * agrees with the columns `deriveStructuredFrontmatterFields` derives from it. + * The legacy `alwaysApply` spelling is folded into `always-apply`. + * + * Key order is preserved: a flag is rewritten in place rather than appended, + * so re-cleaning an unchanged SKILL.md is byte-identical for callers that + * detect drift by comparing serialized bags. + */ +export function toCleanFrontmatter(parsed: ParsedSkillMarkdown): Record { + const clean: Record = {}; + for (const [key, value] of Object.entries(parsed.frontmatter ?? {})) { + if (key === 'name' || key === 'description') { + continue; + } + const flag = SKILL_BOOLEAN_FLAG_BY_KEY.get(key); + if (!flag) { + clean[key] = value; + continue; + } + const resolved = parsed[flag.column]; + if (resolved !== undefined && !(flag.key in clean)) { + clean[flag.key] = resolved; + } + } + return clean; +} diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index d0c8e946a6..769bc8f28b 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -308,6 +308,94 @@ describe('createGitHubSkillSyncRunner', () => { ); }); + it('mirrors user-invocable and disable-model-invocation into the synced frontmatter', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\nuser-invocable: false\ndisable-model-invocation: true\n---\nBody', + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + frontmatter: { 'user-invocable': false, 'disable-model-invocation': true }, + }), + ); + }); + + it('syncs quoted invocation booleans instead of failing strict frontmatter validation', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\nuser-invocable: "false"\n---\nBody', + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + frontmatter: { 'user-invocable': false }, + }), + ); + }); + + it('drops a mid-edit invocation-flag placeholder rather than failing the source', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\nuser-invocable:\n---\nBody', + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + frontmatter: {}, + }), + ); + }); + + it('keeps a flag whose YAML value continues on the next line', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\nalways-apply:\n true\nuser-invocable:\n false\n---\nBody', + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('completed'); + expect(deps.createSkill).toHaveBeenCalledWith( + expect.objectContaining({ + alwaysApply: true, + frontmatter: { 'always-apply': true, 'user-invocable': false }, + }), + ); + }); + + it('marks a source failed when an invocation flag carries a non-boolean value', async () => { + const deps = createDeps({ + fetchFn: githubFetch( + '---\nname: research\ndescription: Research things\ndisable-model-invocation: yes\n---\nBody', + ), + }); + const runner = createGitHubSkillSyncRunner(deps); + const result = await runner.runOnce(); + + expect(result.status).toBe('failed'); + expect(deps.createSkill).not.toHaveBeenCalled(); + expect(deps.upsertStatus).toHaveBeenLastCalledWith( + expect.objectContaining({ + status: 'failed', + errorCode: 'SKILL_PARSE_FAILED', + }), + ); + }); + it('fails duplicate discovered skill names before publishing partial mirrors', async () => { const duplicateFetch = jest.fn(async (input: RequestInfo | URL) => { const url = input.toString(); diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 5af41921f3..1b81caa7eb 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -22,8 +22,8 @@ import type { SkillSyncStatusInput, } from '@librechat/data-schemas'; import type { SkillSyncConfig, SkillSyncGitHubSourceConfig } from 'librechat-data-provider'; +import { parseSkillMarkdown, toCleanFrontmatter } from '../parse'; import { DEFAULT_SKILL_IMPORT_LIMITS } from '../limits'; -import { parseSkillMarkdown } from '../parse'; const GITHUB_API_BASE = 'https://api.github.com'; const SYSTEM_AUTHOR_ID = new Types.ObjectId('000000000000000000000000'); @@ -331,31 +331,6 @@ function guessMimeType(filename: string): string { return mimeMap[ext] ?? 'application/octet-stream'; } -function toCleanFrontmatter( - frontmatter: Record | undefined, -): Record { - if (!frontmatter) { - return {}; - } - const clean = { ...frontmatter }; - delete clean.name; - delete clean.description; - // Drop a placeholder/non-boolean always-apply (e.g. `always-apply:` or - // `always-apply: # TODO`, which js-yaml yields as null). Apply the same - // cleanup to the accepted `alwaysApply` alias so a malformed alias does not - // survive after the canonical key has already supplied the effective flag. - // The boolean is already captured in the dedicated alwaysApply field, and - // persisting a null here would leave ambiguous/invalid frontmatter on the - // synced skill. - if ('always-apply' in clean && typeof clean['always-apply'] !== 'boolean') { - delete clean['always-apply']; - } - if ('alwaysApply' in clean && typeof clean.alwaysApply !== 'boolean') { - delete clean.alwaysApply; - } - return clean; -} - function getLimitMegabytes(bytes: number): number { return Math.round(bytes / 1024 / 1024); } @@ -845,7 +820,7 @@ async function prepareRemoteSkill(params: { name: parsed.name || fallbackName, description: parsed.description || parsed.name || fallbackName, body: skillMdContent, - frontmatter: toCleanFrontmatter(parsed.frontmatter), + frontmatter: toCleanFrontmatter(parsed), alwaysApply: parsed.alwaysApply, source: PROVIDER, sourceMetadata, diff --git a/packages/data-schemas/src/index.ts b/packages/data-schemas/src/index.ts index 85ff1b86fc..799dfbf05b 100644 --- a/packages/data-schemas/src/index.ts +++ b/packages/data-schemas/src/index.ts @@ -16,10 +16,12 @@ export { defaultRate, createTxMethods, permissionBitSupersets, + SKILL_BOOLEAN_FLAGS, partitionIssues, validateSkillName, validateSkillBody, validateRelativePath, + pickValidFrontmatter, inferSkillFileCategory, validateSkillFrontmatter, validateSkillDescription, diff --git a/packages/data-schemas/src/methods/index.ts b/packages/data-schemas/src/methods/index.ts index e21115b8c0..1ae350a138 100644 --- a/packages/data-schemas/src/methods/index.ts +++ b/packages/data-schemas/src/methods/index.ts @@ -73,9 +73,11 @@ import { createPromptMethods, type PromptMethods, type PromptDeps } from './prom import { createSkillMethods, partitionIssues, + SKILL_BOOLEAN_FLAGS, validateSkillName, validateSkillBody, validateRelativePath, + pickValidFrontmatter, validateSkillFrontmatter, validateSkillDescription, deriveStructuredFrontmatterFields, @@ -90,6 +92,8 @@ import { type ListSkillsByAccessResult, type UpdateSkillResult, type ValidationIssue, + type SkillBooleanFlag, + type SkillBooleanColumn, } from './skill'; import { createSkillSyncMethods, type SkillSyncMethods } from './skillSync'; import type { @@ -106,10 +110,12 @@ export { RoleConflictError, DEFAULT_REFRESH_TOKEN_EXPIRY, DEFAULT_SESSION_EXPIRY export { tokenValues, cacheTokenValues, premiumTokenValues, defaultRate, createTxMethods }; export { permissionBitSupersets }; export { + SKILL_BOOLEAN_FLAGS, partitionIssues, validateSkillName, validateSkillBody, validateRelativePath, + pickValidFrontmatter, validateSkillFrontmatter, validateSkillDescription, deriveStructuredFrontmatterFields, @@ -338,6 +344,8 @@ export type { ListSkillsByAccessResult, UpdateSkillResult, ValidationIssue, + SkillBooleanFlag, + SkillBooleanColumn, SkillSyncStatusInput, SkillSyncCredentialSummary, UpsertSkillSyncCredentialInput, diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 0cd12f9e9d..8f8db8be35 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -14,6 +14,7 @@ import { validateSkillFrontmatter, validateAlwaysApply, validateRelativePath, + pickValidFrontmatter, inferSkillFileCategory, filterExistingSkillIds, deriveStructuredFrontmatterFields, @@ -353,6 +354,71 @@ describe('skill validation helpers', () => { }); }); + describe('pickValidFrontmatter', () => { + it('keeps every entry the strict validator accepts', () => { + const frontmatter = { + 'when-to-use': 'When the user needs a demo.', + 'allowed-tools': ['read', 'write'], + 'user-invocable': false, + 'disable-model-invocation': true, + 'always-apply': true, + metadata: { owner: 'data-team' }, + }; + expect(pickValidFrontmatter(frontmatter)).toEqual(frontmatter); + expect(validateSkillFrontmatter(pickValidFrontmatter(frontmatter))).toEqual([]); + }); + + it('drops unknown keys instead of rejecting the whole bag', () => { + expect(pickValidFrontmatter({ 'user-invocable': false, icon: '🚀', tags: ['a'] })).toEqual({ + 'user-invocable': false, + }); + }); + + it('drops recognized keys whose value fails its declared kind', () => { + expect( + pickValidFrontmatter({ + 'user-invocable': false, + version: 1.0, + effort: ['high'], + 'allowed-tools': [1, 2], + }), + ).toEqual({ 'user-invocable': false }); + }); + + it('drops hooks / metadata that are not plain JSON-safe objects', () => { + expect(pickValidFrontmatter({ hooks: 'echo hi' })).toEqual({}); + expect( + pickValidFrontmatter({ metadata: { a: { b: { c: { d: { e: { f: 'deep' } } } } } } }), + ).toEqual({}); + }); + + it('returns an empty bag for non-object input', () => { + expect(pickValidFrontmatter(undefined)).toEqual({}); + expect(pickValidFrontmatter(null)).toEqual({}); + expect(pickValidFrontmatter([])).toEqual({}); + expect(pickValidFrontmatter('name: demo')).toEqual({}); + }); + + it('never yields a bag the strict validator would reject', () => { + const messy = { + name: 'demo-skill', + 'user-invocable': 'yes', + 'disable-model-invocation': null, + unknown: 1, + hooks: [], + license: 'MIT', + }; + expect(validateSkillFrontmatter(pickValidFrontmatter(messy))).toEqual([]); + expect(pickValidFrontmatter(messy)).toEqual({ name: 'demo-skill', license: 'MIT' }); + }); + + it('preserves key order so serialized-bag drift checks stay stable', () => { + expect( + Object.keys(pickValidFrontmatter({ license: 'MIT', icon: 'x', 'user-invocable': true })), + ).toEqual(['license', 'user-invocable']); + }); + }); + describe('validateAlwaysApply', () => { it('accepts undefined and booleans (undefined = no change)', () => { expect(validateAlwaysApply(undefined)).toEqual([]); @@ -779,6 +845,186 @@ describe('Skill CRUD methods', () => { expect(updated.skill.allowedTools).toBeUndefined(); }); + /** + * The shipping create/edit forms send `body` with no structured + * `frontmatter`, so the SKILL.md text is the only place a user can declare + * these flags. Before this cascade existed only `always-apply` was read out + * of the body and the other two silently kept their defaults. + */ + describe('invocation-mode columns derived from the SKILL.md body', () => { + const bodyWithFlags = [ + '---', + 'name: body-flags', + 'description: A skill that restricts its invocation channels.', + 'always-apply: true', + 'user-invocable: false', + 'disable-model-invocation: true', + '---', + 'Body.', + ].join('\n'); + + it('createSkill honors flags declared only in the body', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'body-flags', body: bodyWithFlags, frontmatter: undefined }), + ); + expect(skill.alwaysApply).toBe(true); + expect(skill.userInvocable).toBe(false); + expect(skill.disableModelInvocation).toBe(true); + + const reloaded = await methods.getSkillById(skill._id); + expect(reloaded?.userInvocable).toBe(false); + expect(reloaded?.disableModelInvocation).toBe(true); + }); + + it('createSkill lets an explicit frontmatter bag win over the body', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'bag-wins', + body: bodyWithFlags, + frontmatter: { 'user-invocable': true }, + }), + ); + expect(skill.userInvocable).toBe(true); + /* Not declared in the bag, so the body still supplies it. */ + expect(skill.disableModelInvocation).toBe(true); + }); + + it('createSkill rejects a non-boolean flag in the body', async () => { + await expect( + methods.createSkill( + makeSkillInput({ + name: 'body-typo', + body: '---\nname: body-typo\ndescription: A demo skill.\nuser-invocable: yes\n---\n\nBody.', + frontmatter: undefined, + }), + ), + ).rejects.toMatchObject({ + code: 'SKILL_VALIDATION_FAILED', + issues: expect.arrayContaining([ + expect.objectContaining({ field: 'body.frontmatter.user-invocable' }), + ]), + }); + }); + + it('createSkill accepts a body typo the frontmatter bag overrides', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'body-typo-overridden', + body: '---\nname: body-typo-overridden\ndescription: A demo skill.\nuser-invocable: yes\n---\n\nBody.', + frontmatter: { 'user-invocable': false }, + }), + ); + expect(skill.userInvocable).toBe(false); + }); + + it('createSkill ignores a flag nested under another frontmatter key', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'nested-flag', + body: '---\nname: nested-flag\ndescription: A demo skill.\nmetadata:\n user-invocable: nonsense\n---\n\nBody.', + frontmatter: undefined, + }), + ); + expect(skill.userInvocable).toBe(true); + }); + + it('updateSkill picks up flags added to the body', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'body-add-flags' })); + expect(skill.userInvocable).toBe(true); + + const updated = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + body: '---\nname: body-add-flags\ndescription: A demo skill.\nuser-invocable: false\n---\n\nBody.', + }, + }); + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + expect(updated.skill.userInvocable).toBe(false); + }); + + it('updateSkill releases a restriction when the body drops the flag line', async () => { + /* Regression for the sticky-column trap: an imported skill restricted to + manual-only had no way back — the edit UI sends `body` only, so the + column stayed `false` while the SKILL.md text no longer said so. */ + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'body-release', body: bodyWithFlags, frontmatter: undefined }), + ); + expect(skill.userInvocable).toBe(false); + expect(skill.disableModelInvocation).toBe(true); + + const updated = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + body: '---\nname: body-release\ndescription: A demo skill.\n---\n\nBody.', + }, + }); + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + /* Unset, which every runtime gate reads as the schema default. */ + expect(updated.skill.userInvocable).toBeUndefined(); + expect(updated.skill.disableModelInvocation).toBeUndefined(); + }); + + it('updateSkill leaves the columns alone when the update touches neither body nor frontmatter', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'untouched-columns', body: bodyWithFlags, frontmatter: undefined }), + ); + + const updated = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { category: 'research' }, + }); + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + expect(updated.skill.userInvocable).toBe(false); + expect(updated.skill.disableModelInvocation).toBe(true); + }); + + it('updateSkill keeps allowedTools when only the body changes', async () => { + /* The body scan reads booleans, not YAML sequences, so a body-only update + must not drop a tool list it cannot re-read. */ + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'keeps-tools', + frontmatter: { 'allowed-tools': ['execute_code'] }, + }), + ); + expect(skill.allowedTools).toEqual(['execute_code']); + + const updated = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { body: '---\nname: keeps-tools\ndescription: A demo skill.\n---\n\nBody.' }, + }); + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + expect(updated.skill.allowedTools).toEqual(['execute_code']); + }); + + it('updateSkill rejects a non-boolean flag introduced by a body edit', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'body-edit-typo' })); + + await expect( + methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + body: '---\nname: body-edit-typo\ndescription: A demo skill.\ndisable-model-invocation: 1\n---\n\nBody.', + }, + }), + ).rejects.toMatchObject({ + code: 'SKILL_VALIDATION_FAILED', + issues: expect.arrayContaining([ + expect.objectContaining({ field: 'body.frontmatter.disable-model-invocation' }), + ]), + }); + }); + }); + it('backfills legacy skills from frontmatter when columns are unset (getSkillByName)', async () => { /* Simulate a pre-Phase-6 skill: it has `user-invocable` / `disable-model-invocation` / `allowed-tools` set in frontmatter diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 4b0632a336..700b910744 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -234,6 +234,33 @@ export function validateAlwaysApply(alwaysApply: unknown): ValidationIssue[] { return []; } +/** Column on a skill document that mirrors a boolean frontmatter flag. */ +export type SkillBooleanColumn = 'alwaysApply' | 'userInvocable' | 'disableModelInvocation'; + +export type SkillBooleanFlag = { + /** Column the flag is mirrored onto. */ + column: SkillBooleanColumn; + /** Canonical kebab-case frontmatter key. */ + key: string; + /** + * Legacy spellings accepted on read and normalized to `key` on write. + * Consulted only when the canonical key is absent. + */ + aliases: readonly string[]; +}; + +/** + * Boolean frontmatter flags mirrored onto first-class columns. Shared with the + * SKILL.md parser in `@librechat/api` so the parser, the body extractor, and + * the column derivation can't disagree about which keys exist or which column + * each one feeds. + */ +export const SKILL_BOOLEAN_FLAGS: readonly SkillBooleanFlag[] = [ + { column: 'alwaysApply', key: 'always-apply', aliases: ['alwaysApply'] }, + { column: 'userInvocable', key: 'user-invocable', aliases: [] }, + { column: 'disableModelInvocation', key: 'disable-model-invocation', aliases: [] }, +]; + /** * 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 @@ -343,6 +370,48 @@ function isJsonSafe(value: unknown, depth: number): boolean { return false; } +/** + * Evaluate one frontmatter entry against the strict allowlist and its declared + * kind, returning `null` when the entry is acceptable. Shared by + * `validateSkillFrontmatter` (which reports issues) and + * `pickValidFrontmatter` (which drops them) so the two can never disagree on + * what a valid entry is. + */ +function checkFrontmatterEntry(key: string, value: unknown): ValidationIssue | null { + if (!ALLOWED_FRONTMATTER_KEYS.has(key)) { + return { + field: `frontmatter.${key}`, + code: 'UNKNOWN_KEY', + message: `"${key}" is not a recognized frontmatter key`, + }; + } + + if (key === 'hooks' || key === 'metadata') { + if (!isPlainObject(value) || !isJsonSafe(value, 0)) { + return { + field: `frontmatter.${key}`, + code: 'INVALID_SHAPE', + message: `"${key}" must be a plain JSON-safe object (max depth ${FRONTMATTER_MAX_DEPTH}, max string ${FRONTMATTER_MAX_STRING})`, + }; + } + return null; + } + + const expected = FRONTMATTER_KIND[key]; + if (!expected) { + return null; + } + const kinds = Array.isArray(expected) ? expected : [expected]; + if (!kinds.some((kind) => matchesKind(value, kind))) { + return { + field: `frontmatter.${key}`, + code: 'INVALID_TYPE', + message: `"${key}" must be ${kinds.join(' or ')}`, + }; + } + return null; +} + /** * Validate a skill's structured YAML frontmatter. Strict mode: unknown keys * are rejected so any expansion of the allowed set is an intentional code @@ -366,42 +435,44 @@ export function validateSkillFrontmatter(frontmatter: unknown): ValidationIssue[ const issues: ValidationIssue[] = []; for (const [key, value] of Object.entries(frontmatter)) { - if (!ALLOWED_FRONTMATTER_KEYS.has(key)) { - issues.push({ - field: `frontmatter.${key}`, - code: 'UNKNOWN_KEY', - message: `"${key}" is not a recognized frontmatter key`, - }); - continue; - } - - if (key === 'hooks' || key === 'metadata') { - if (!isPlainObject(value) || !isJsonSafe(value, 0)) { - issues.push({ - field: `frontmatter.${key}`, - code: 'INVALID_SHAPE', - message: `"${key}" must be a plain JSON-safe object (max depth ${FRONTMATTER_MAX_DEPTH}, max string ${FRONTMATTER_MAX_STRING})`, - }); - } - continue; - } - - const expected = FRONTMATTER_KIND[key]; - if (!expected) { - continue; - } - const kinds = Array.isArray(expected) ? expected : [expected]; - if (!kinds.some((kind) => matchesKind(value, kind))) { - issues.push({ - field: `frontmatter.${key}`, - code: 'INVALID_TYPE', - message: `"${key}" must be ${kinds.join(' or ')}`, - }); + const issue = checkFrontmatterEntry(key, value); + if (issue) { + issues.push(issue); } } return issues; } +/** + * Narrow a frontmatter bag to exactly the entries `validateSkillFrontmatter` + * accepts, dropping unknown keys and values that fail their declared kind. + * + * For ingestion paths that accept files authored outside LibreChat (skill + * import), where the bag is a byproduct of the upload rather than something + * the uploader typed: a stray `version: 1.0` or a bespoke `icon:` key must + * not fail an otherwise valid import, and neither may it reach `createSkill` + * and trip strict validation there. Fields whose value is load-bearing for + * behavior (the invocation-mode booleans) are resolved and reported by the + * caller's own parser before this filter runs, so a malformed one still + * surfaces as an error rather than being quietly dropped here. + * + * Admin-authored paths (GitHub sync, deployment skills) deliberately skip + * this and keep the strict validator's hard failure — a typo in a curated + * source should be loud. + */ +export function pickValidFrontmatter(frontmatter: unknown): Record { + if (!isPlainObject(frontmatter)) { + return {}; + } + const picked: Record = {}; + for (const [key, value] of Object.entries(frontmatter)) { + if (checkFrontmatterEntry(key, value) === null) { + picked[key] = value; + } + } + return picked; +} + export function validateRelativePath(relativePath: unknown): ValidationIssue[] { const issues: ValidationIssue[] = []; if (typeof relativePath !== 'string' || relativePath.length === 0) { @@ -700,97 +771,184 @@ type BodyAlwaysApplyResult = | { status: 'valid'; value: boolean } | { status: 'invalid' }; -/** - * Extractor for the `always-apply` / `alwaysApply` flag sitting inside a SKILL.md body's - * YAML frontmatter block. The REST edit flow lets users rewrite the full - * SKILL.md text via `update.body` without a structured `frontmatter` - * object, so this is the only signal we have for "user flipped - * `always-apply:` or `alwaysApply:` inline in their editor". - * - * Returns a discriminated union so callers can tell: - * - `absent` — no always-apply key (leave column alone; could be - * "user removed the flag" or "user hasn't written it yet" — both - * resolve to no-op). An empty value (`always-apply:` with nothing - * after the colon) is also treated as absent to allow mid-edit - * placeholder states without rejecting a save. - * - `valid` — parsed cleanly as `true` / `false` (case-insensitive, - * quote-tolerant, YAML inline-comment-tolerant). - * - `invalid` — key is present with a non-empty value that isn't a - * recognizable boolean (e.g. `tru`, `yes`, `1`). Validation rejects - * this rather than silently ignoring so `always-apply: tru` typos - * surface as 400s instead of drifting the column from what the - * saved SKILL.md text says. When both forms are present, the canonical - * `always-apply` form wins because existing files may already rely on it. - */ -function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyResult { +/** Body-derived state for every boolean flag mirrored onto a column. */ +type BodyFlagResults = Record; + +const BODY_FLAG_BY_KEY = new Map( + SKILL_BOOLEAN_FLAGS.flatMap((flag) => + [flag.key, ...flag.aliases].map((key) => [key.toLowerCase(), flag] as const), + ), +); + +/** Isolate a SKILL.md body's leading YAML frontmatter block, or `null`. */ +function extractBodyFrontmatterBlock(body: string | undefined): string | null { if (typeof body !== 'string' || body.length === 0) { - return { status: 'absent' }; + return null; } const trimmed = body.trim(); if (!trimmed.startsWith('---')) { - return { status: 'absent' }; + return null; } const after = trimmed.slice(3); const closingIdx = after.indexOf('\n---'); if (closingIdx === -1) { + return null; + } + return after.slice(0, closingIdx); +} + +function readBodyFlagValue(rawValue: string): BodyAlwaysApplyResult { + /* Strip the YAML inline comment BEFORE unquoting — a line like + `always-apply: "true" # note` has both, and handling whole-line quoting + first would leave `"true"` behind, which parses as invalid. */ + let value = stripYamlTrailingComment(rawValue.trim()).trim(); + if (value === '') { return { status: 'absent' }; } - const block = after.slice(0, closingIdx); - let aliasResult: BodyAlwaysApplyResult | undefined; + if ( + value.length >= 2 && + ((value[0] === '"' && value[value.length - 1] === '"') || + (value[0] === "'" && value[value.length - 1] === "'")) + ) { + value = value.slice(1, -1).trim(); + } + if (value === '') { + return { status: 'absent' }; + } + const lowered = value.toLowerCase(); + if (lowered === 'true') { + return { status: 'valid', value: true }; + } + if (lowered === 'false') { + return { status: 'valid', value: false }; + } + return { status: 'invalid' }; +} + +/** + * Extractor for the boolean invocation-mode flags sitting inside a SKILL.md + * body's YAML frontmatter block. The REST edit flow lets users rewrite the + * full SKILL.md text via `update.body` without a structured `frontmatter` + * object, so this is the only signal we have for "user flipped + * `user-invocable:` inline in their editor". + * + * Each flag resolves to a discriminated union so callers can tell: + * - `absent` — key not present (or present with an empty value, a mid-edit + * placeholder that must not reject a save). Treated as a declaration that + * the flag is off, so removing a line returns the column to its default. + * - `valid` — parsed cleanly as `true` / `false` (case-insensitive, + * quote-tolerant, YAML inline-comment-tolerant). + * - `invalid` — present with a non-empty value that isn't a recognizable + * boolean (`tru`, `yes`, `1`). Validation rejects this rather than silently + * ignoring it, so typos surface as 400s instead of drifting the column away + * from what the saved SKILL.md text says. + * + * The first canonical spelling wins; a legacy alias (`alwaysApply`) is only + * consulted when the canonical key never appears. Indented lines are skipped: + * a nested mapping that reuses a flag name (`metadata:` → ` user-invocable:`) + * is not a top-level declaration and must not be read as one. + */ +function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagResults { + const results: BodyFlagResults = { + alwaysApply: { status: 'absent' }, + userInvocable: { status: 'absent' }, + disableModelInvocation: { status: 'absent' }, + }; + const block = extractBodyFrontmatterBlock(body); + if (block === null) { + return results; + } + const canonical = new Map(); + const aliased = new Map(); for (const line of block.split('\n')) { + if (line.length === 0 || line[0] === ' ' || line[0] === '\t') { + continue; + } const colon = line.indexOf(':'); if (colon === -1) { continue; } - const key = line.slice(0, colon).trim(); - const normalizedKey = key.toLowerCase(); - if (normalizedKey !== 'always-apply' && normalizedKey !== 'alwaysapply') { + const key = line.slice(0, colon).trim().toLowerCase(); + const flag = BODY_FLAG_BY_KEY.get(key); + if (!flag) { continue; } - // Strip the YAML inline comment BEFORE unquoting — a line like - // `always-apply: "true" # note` has both, and if we only handled - // whole-line quoting first, the quoted branch wouldn't match and - // the comment-strip would leave `"true"` which parses as invalid. - let value = stripYamlTrailingComment(line.slice(colon + 1).trim()).trim(); - if (value === '') { - const result: BodyAlwaysApplyResult = { status: 'absent' }; - if (normalizedKey === 'always-apply') { - return result; + const result = readBodyFlagValue(line.slice(colon + 1)); + if (key === flag.key) { + if (!canonical.has(flag.column)) { + canonical.set(flag.column, result); } - aliasResult = result; continue; } - if ( - value.length >= 2 && - ((value[0] === '"' && value[value.length - 1] === '"') || - (value[0] === "'" && value[value.length - 1] === "'")) - ) { - value = value.slice(1, -1); - } - value = value.trim(); - if (value === '') { - const result: BodyAlwaysApplyResult = { status: 'absent' }; - if (normalizedKey === 'always-apply') { - return result; - } - aliasResult = result; - continue; - } - const lowered = value.toLowerCase(); - let result: BodyAlwaysApplyResult; - if (lowered === 'true') { - result = { status: 'valid', value: true }; - } else if (lowered === 'false') { - result = { status: 'valid', value: false }; - } else { - result = { status: 'invalid' }; - } - if (normalizedKey === 'always-apply') { - return result; - } - aliasResult = result; + aliased.set(flag.column, result); } - return aliasResult ?? { status: 'absent' }; + for (const flag of SKILL_BOOLEAN_FLAGS) { + const resolved = canonical.get(flag.column) ?? aliased.get(flag.column); + if (resolved) { + results[flag.column] = resolved; + } + } + return results; +} + +function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyResult { + return extractBooleanFlagsFromBody(body).alwaysApply; +} + +/** + * Columns whose only inputs are the frontmatter bag and the body's own + * frontmatter. `alwaysApply` is excluded: it additionally accepts an explicit + * top-level input and is a non-nullable column, so it runs its own cascade. + */ +const BODY_DERIVED_COLUMNS = ['userInvocable', 'disableModelInvocation'] as const; + +/** + * Resolve one boolean column from the two sources that can carry it, in + * precedence order: an explicit key in the structured `frontmatter` bag, then + * the SKILL.md body's own frontmatter. `undefined` means "declared nowhere", + * which callers turn into the schema default. + */ +function resolveBodyDerivedColumn( + column: (typeof BODY_DERIVED_COLUMNS)[number], + bagDerived: { userInvocable?: boolean; disableModelInvocation?: boolean } | undefined, + bodyFlags: BodyFlagResults | undefined, +): boolean | undefined { + const fromBag = bagDerived?.[column]; + if (typeof fromBag === 'boolean') { + return fromBag; + } + const fromBody = bodyFlags?.[column]; + return fromBody?.status === 'valid' ? fromBody.value : undefined; +} + +/** + * Report body-declared flags whose value isn't a boolean, skipping any the + * caller is already overriding through the structured `frontmatter` bag. + */ +function validateBodyDerivedColumns( + frontmatter: Record | undefined, + bodyFlags: BodyFlagResults | undefined, +): ValidationIssue[] { + if (!bodyFlags) { + return []; + } + const bagDerived = deriveStructuredFrontmatterFields(frontmatter); + const issues: ValidationIssue[] = []; + for (const flag of SKILL_BOOLEAN_FLAGS) { + if (flag.column === 'alwaysApply') { + continue; + } + const column = flag.column as (typeof BODY_DERIVED_COLUMNS)[number]; + if (bodyFlags[column].status !== 'invalid' || typeof bagDerived[column] === 'boolean') { + continue; + } + issues.push({ + field: `body.frontmatter.${flag.key}`, + code: 'INVALID_TYPE', + message: `"${flag.key}" in SKILL.md frontmatter must be a boolean (true or false)`, + }); + } + return issues; } /** @@ -1024,11 +1182,11 @@ export function createSkillMethods( } async function createSkill(data: CreateSkillInput): Promise { - /* 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. */ - const bodyAlwaysApply = - data.body !== undefined ? extractAlwaysApplyFromBody(data.body) : undefined; + /* Parse the body's flag declarations once — reused for validation (below) + and for the derivation cascades. Avoids parsing the same YAML + frontmatter block twice per create. */ + const bodyFlags = data.body !== undefined ? extractBooleanFlagsFromBody(data.body) : undefined; + const bodyAlwaysApply = bodyFlags?.alwaysApply; const issues: ValidationIssue[] = [ ...validateSkillName(data.name), ...validateSkillDescription(data.description), @@ -1036,6 +1194,7 @@ export function createSkillMethods( ...validateSkillDisplayTitle(data.displayTitle), ...validateSkillFrontmatter(data.frontmatter), ...validateAlwaysApply(data.alwaysApply), + ...validateBodyDerivedColumns(data.frontmatter, bodyFlags), ]; /* Body-level `always-apply:` only needs to be well-formed when a higher-precedence source won't override it (see @@ -1084,6 +1243,20 @@ export function createSkillMethods( } const derived = deriveStructuredFrontmatterFields(data.frontmatter); + /** + * A caller may declare the invocation-mode flags in the structured bag, in + * the SKILL.md body's own frontmatter, or both — the UI's create form sends + * only `body`. Resolve each column from whichever source carries it so a + * flag written inline is honored the same way `always-apply` already is. + * Keys the bag declares still win, so `derived` is spread last. + */ + const bodyDerived: { userInvocable?: boolean; disableModelInvocation?: boolean } = {}; + for (const column of BODY_DERIVED_COLUMNS) { + const resolved = resolveBodyDerivedColumn(column, derived, bodyFlags); + if (resolved !== undefined) { + bodyDerived[column] = resolved; + } + } const doc = await Skill.create({ name: data.name, displayTitle: data.displayTitle, @@ -1105,6 +1278,7 @@ export function createSkillMethods( bodyAlwaysApply, ), tenantId: data.tenantId, + ...bodyDerived, ...derived, }); return { @@ -1346,12 +1520,12 @@ export function createSkillMethods( return { status: 'not_found' }; } - /* Parse body's always-apply status once — reused for validation - (precedence-aware, below) and the derivation cascade further - down. Avoids parsing the same YAML frontmatter block twice per - update. */ - const bodyAlwaysApply = - update.body !== undefined ? extractAlwaysApplyFromBody(update.body) : undefined; + /* Parse the body's flag declarations once — reused for validation + (precedence-aware, below) and the derivation cascades further down. + Avoids parsing the same YAML frontmatter block twice per update. */ + const bodyFlags = + update.body !== undefined ? extractBooleanFlagsFromBody(update.body) : undefined; + const bodyAlwaysApply = bodyFlags?.alwaysApply; const issues: ValidationIssue[] = []; if (update.name !== undefined) issues.push(...validateSkillName(update.name)); if (update.description !== undefined) @@ -1362,6 +1536,7 @@ export function createSkillMethods( if (update.frontmatter !== undefined) issues.push(...validateSkillFrontmatter(update.frontmatter)); if (update.alwaysApply !== undefined) issues.push(...validateAlwaysApply(update.alwaysApply)); + issues.push(...validateBodyDerivedColumns(update.frontmatter, bodyFlags)); /* Body-level `always-apply:` only needs to be well-formed when a higher-precedence source won't override it (see `resolveAlwaysApplyFromInput` for precedence). Rejecting a typo @@ -1397,21 +1572,41 @@ export function createSkillMethods( if (update.body !== undefined) setPayload.body = update.body; if (update.source !== undefined) setPayload.source = update.source; if (update.sourceMetadata !== undefined) setPayload.sourceMetadata = update.sourceMetadata; + const bagDerived = + update.frontmatter !== undefined + ? deriveStructuredFrontmatterFields(update.frontmatter) + : undefined; if (update.frontmatter !== undefined) { setPayload.frontmatter = update.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. + * `allowedTools` tracks the frontmatter bag alone — the body scan reads + * boolean flags, not YAML sequences — so a bag that omits `allowed-tools` + * unsets the column, while a body-only update leaves it untouched rather + * than dropping a list it cannot re-read. */ - const derived = deriveStructuredFrontmatterFields(update.frontmatter); - for (const key of ['disableModelInvocation', 'userInvocable', 'allowedTools'] as const) { - if (derived[key] !== undefined) { - setPayload[key] = derived[key]; - } else { - unsetPayload[key] = ''; - } + if (bagDerived?.allowedTools !== undefined) { + setPayload.allowedTools = bagDerived.allowedTools; + } else { + unsetPayload.allowedTools = ''; + } + } + /** + * Boolean invocation-mode columns follow whichever source the update + * carries: a key in the structured bag wins, then the SKILL.md body's own + * frontmatter (the only signal the UI edit flow sends). When neither + * declares the flag but the update did supply one of those sources, the + * column is unset back to its schema default — removing + * `disable-model-invocation:` from a SKILL.md re-enables model invocation, + * mirroring how a removed `always-apply:` line stops auto-priming. Updates + * touching neither `frontmatter` nor `body` leave the columns alone. + */ + const declaresColumns = update.frontmatter !== undefined || update.body !== undefined; + for (const column of BODY_DERIVED_COLUMNS) { + const resolved = resolveBodyDerivedColumn(column, bagDerived, bodyFlags); + if (resolved !== undefined) { + setPayload[column] = resolved; + } else if (declaresColumns) { + unsetPayload[column] = ''; } } if (update.category !== undefined) setPayload.category = update.category;