diff --git a/packages/api/src/skills/__tests__/import.integration.spec.ts b/packages/api/src/skills/__tests__/import.db.spec.ts similarity index 100% rename from packages/api/src/skills/__tests__/import.integration.spec.ts rename to packages/api/src/skills/__tests__/import.db.spec.ts diff --git a/packages/api/src/skills/__tests__/parity.db.spec.ts b/packages/api/src/skills/__tests__/parity.db.spec.ts new file mode 100644 index 0000000000..a0205a17d2 --- /dev/null +++ b/packages/api/src/skills/__tests__/parity.db.spec.ts @@ -0,0 +1,167 @@ +import mongoose, { Types } from 'mongoose'; +import { MongoMemoryServer } from 'mongodb-memory-server'; +import { + logger, + createModels, + createMethods, + pickValidFrontmatter, + type AllMethods, +} from '@librechat/data-schemas'; +import { parseSkillMarkdown, toCleanFrontmatter } from '../parse'; + +logger.silent = true; + +/** + * The invariant this PR rests on: the same SKILL.md text must produce the same + * invocation-mode columns whether it arrives as an upload or is pasted into the + * skill editor. Those two routes read frontmatter differently — the upload path + * runs js-yaml (`parseSkillMarkdown`), while a body-only save is read by the + * line scanner inside `createSkill` — so nothing but a shared corpus keeps them + * from drifting apart on the next change. + * + * Each row is fed through both routes and the resulting columns compared. Add a + * row here for any frontmatter shape a bug report turns up. + */ + +type Columns = { + alwaysApply?: boolean; + userInvocable?: boolean; + disableModelInvocation?: boolean; + rejected?: true; +}; + +let mongoServer: MongoMemoryServer; +let methods: AllMethods; + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + createModels(mongoose); + methods = createMethods(mongoose); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +const DESCRIPTION = 'A skill used by the frontmatter parity corpus.'; + +function bodyFor(name: string, frontmatterLines: string): string { + return `---\nname: ${name}\ndescription: ${DESCRIPTION}\n${frontmatterLines}\n---\n\nBody.`; +} + +function columnsOf(skill: { + alwaysApply?: boolean; + userInvocable?: boolean; + disableModelInvocation?: boolean; +}): Columns { + return { + alwaysApply: skill.alwaysApply, + userInvocable: skill.userInvocable, + disableModelInvocation: skill.disableModelInvocation, + }; +} + +/** The upload route: js-yaml parse, cleaned bag, then `createSkill`. */ +async function viaImport(name: string, body: string): Promise { + const parsed = parseSkillMarkdown(body); + if (parsed.parseError || parsed.invalidBooleans.length > 0) { + return { rejected: true }; + } + const { skill } = await methods.createSkill({ + name: `${name}-import`, + description: DESCRIPTION, + body, + frontmatter: pickValidFrontmatter(toCleanFrontmatter(parsed)), + alwaysApply: parsed.alwaysApply, + author: new Types.ObjectId(), + authorName: 'Parity', + }); + return columnsOf(skill); +} + +/** The editor route: body only, read by the body scanner inside `createSkill`. */ +async function viaInlineBody(name: string, body: string): Promise { + try { + const { skill } = await methods.createSkill({ + name: `${name}-inline`, + description: DESCRIPTION, + body, + author: new Types.ObjectId(), + authorName: 'Parity', + }); + return columnsOf(skill); + } catch { + return { rejected: true }; + } +} + +const CORPUS: Array<[label: string, frontmatterLines: string]> = [ + ['plain booleans', 'user-invocable: false\ndisable-model-invocation: true'], + ['double-quoted value', 'user-invocable: "false"'], + ['single-quoted value', "user-invocable: 'false'"], + ['inline comment', 'user-invocable: false # off'], + ['quoted value with comment', 'user-invocable: "false" # off'], + ['comment-only value', 'user-invocable: # todo'], + ['empty value', 'user-invocable:'], + ['value on the next line', 'user-invocable:\n false'], + ['uppercase key and value', 'USER-INVOCABLE: FALSE'], + ['spaces before the colon', 'user-invocable : false'], + ['trailing spaces after the value', 'user-invocable: false '], + ['double-quoted key', '"user-invocable": false'], + ['single-quoted key', "'user-invocable': false"], + ['empty quoted value', 'user-invocable: ""'], + [ + 'nested duplicate before the real key', + 'metadata:\n user-invocable: nonsense\nuser-invocable: false', + ], + ['nested key only', 'metadata:\n user-invocable: false'], + ['flow-style nested mapping', 'metadata: {user-invocable: nonsense}'], + ['multi-line plain scalar', 'user-invocable:\n false\n extra'], + ['always-apply alias', 'alwaysApply: true'], + ['both always-apply spellings', 'always-apply: false\nalwaysApply: true'], + ['always-apply on the next line', 'always-apply:\n true'], + ['all three flags', 'always-apply: true\nuser-invocable: false\ndisable-model-invocation: true'], + ['carriage return after the value', 'user-invocable: false\r'], +]; + +describe('frontmatter parity between the upload and inline-body routes', () => { + let index = 0; + + it.each(CORPUS)('%s', async (_label, frontmatterLines) => { + const name = `parity-${index++}`; + const body = bodyFor(name, frontmatterLines); + + expect(await viaInlineBody(name, body)).toEqual(await viaImport(name, body)); + }); + + it('rejects YAML the editor route tolerates, without setting a column either way', async () => { + /* Tabs are illegal for YAML indentation, so js-yaml refuses the file and the + upload is rejected outright, while the line scanner simply sees nothing at + the mapping's indent and saves the text as authored. The asymmetry is in + the rejection, never in the columns — neither route sets a flag — and it + predates this work, since only the upload route ever parsed YAML. */ + const name = 'parity-tab-indent'; + const body = bodyFor(name, 'metadata:\n\tuser-invocable: false'); + + expect(await viaImport(name, body)).toEqual({ rejected: true }); + expect(await viaInlineBody(name, body)).toMatchObject({ + userInvocable: true, + disableModelInvocation: false, + }); + }); + + it('documents the one shape the two routes read differently', async () => { + /* Duplicate keys differing only in case: js-yaml keeps the last after the + keys normalize to one, the line scanner takes the first. The file is + ambiguous by construction and neither reading can release a restriction, + so this is pinned rather than fixed — widening the line scanner into a + second YAML implementation would cost more than it buys. */ + const name = 'parity-case-duplicate'; + const body = bodyFor(name, 'user-invocable: false\nUSER-INVOCABLE: true'); + + expect(await viaImport(name, body)).toMatchObject({ userInvocable: true }); + expect(await viaInlineBody(name, body)).toMatchObject({ userInvocable: false }); + }); +}); diff --git a/packages/api/src/skills/handlers.ts b/packages/api/src/skills/handlers.ts index d4a0f46ed4..a476e522e1 100644 --- a/packages/api/src/skills/handlers.ts +++ b/packages/api/src/skills/handlers.ts @@ -399,7 +399,17 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps): { }); } catch (error) { if (isValidationError(error)) { - return res.status(400).json({ error: 'Validation failed', issues: error.issues }); + return res.status(400).json({ + error: 'Validation failed', + issues: error.issues, + /* The client shows `message` when present and an unhelpful generic + string otherwise, so name the offending field — a rejected + invocation-mode flag is a line in the SKILL.md the author can + only find if we say which one. Mirrors the import handler. */ + message: (error.issues as Array<{ message?: string }>) + ?.map((issue) => issue.message) + .join('; '), + }); } if (isDuplicateKeyError(error)) { return res.status(409).json({ error: 'A skill with this name already exists' }); @@ -508,7 +518,17 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps): { result = await updateSkill({ id, expectedVersion, update }); } catch (error) { if (isValidationError(error)) { - return res.status(400).json({ error: 'Validation failed', issues: error.issues }); + return res.status(400).json({ + error: 'Validation failed', + issues: error.issues, + /* The client shows `message` when present and an unhelpful generic + string otherwise, so name the offending field — a rejected + invocation-mode flag is a line in the SKILL.md the author can + only find if we say which one. Mirrors the import handler. */ + message: (error.issues as Array<{ message?: string }>) + ?.map((issue) => issue.message) + .join('; '), + }); } if (isDuplicateKeyError(error)) { return res.status(409).json({ error: 'A skill with this name already exists' }); diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 94a48a0857..ef5b5d55e2 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -1159,6 +1159,37 @@ describe('Skill CRUD methods', () => { expect(updated.skill.allowedTools).toEqual(['execute_code']); }); + it('lets an unrelated save through when the stored body already had a malformed flag', async () => { + /* Pre-fix import accepted `user-invocable: yes` at 201 and dropped the + flag, so installs have skills whose stored SKILL.md carries exactly + that. The editor resubmits the whole file on every save, so validating + it as if this edit introduced it would leave those skills permanently + unsavable — the user cannot even fix the description. */ + const malformed = + '---\nname: stored-typo\ndescription: A demo skill.\nuser-invocable: yes\n---\n\nBody.'; + const stored = await Skill.create({ + name: 'stored-typo', + description: 'A demo skill.', + body: malformed, + frontmatter: {}, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + }); + + const updated = await methods.updateSkill({ + id: (stored._id as mongoose.Types.ObjectId).toString(), + expectedVersion: 1, + update: { description: 'An edited description.', body: malformed }, + }); + + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + expect(updated.skill.description).toBe('An edited description.'); + }); + it('updateSkill rejects a non-boolean flag introduced by a body edit', async () => { const { skill } = await methods.createSkill(makeSkillInput({ name: 'body-edit-typo' })); diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 1ba498959e..404b8dbab1 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -996,6 +996,7 @@ function resolveBodyDerivedColumn( function validateBodyDerivedColumns( frontmatter: Record | undefined, bodyFlags: BodyFlagResults | undefined, + storedBodyFlags?: BodyFlagResults, ): ValidationIssue[] { if (!bodyFlags) { return []; @@ -1010,6 +1011,10 @@ function validateBodyDerivedColumns( if (bodyFlags[column].status !== 'invalid' || typeof bagDerived[column] === 'boolean') { continue; } + /* Already malformed before this edit — not something to reject now. */ + if (storedBodyFlags?.[column].status === 'invalid') { + continue; + } issues.push({ field: `body.frontmatter.${flag.key}`, code: 'INVALID_TYPE', @@ -1594,6 +1599,37 @@ export function createSkillMethods( const bodyScan = update.body !== undefined ? extractBooleanFlagsFromBody(update.body) : undefined; const bodyAlwaysApply = bodyScan?.alwaysApply; + const Skill = mongoose.models.Skill as Model; + /** + * What the SKILL.md text declared before this edit. Two rules depend on it, + * and both exist because the body reader's silence must never be read as an + * instruction: + * - a body edit may only RELEASE a flag that this same body used to + * declare, so a skill whose flags live only in the frontmatter bag (the + * legacy shape `backfillDerivedFromFrontmatter` serves, and what setting + * flags through the API alone produces) keeps them; + * - a malformed flag already sitting in the stored text is not this edit's + * fault, so it does not block an unrelated save. The editor resubmits the + * whole file on every save, so rejecting it would leave such a skill + * permanently unsavable. + * A key the reader cannot see is therefore invisible on both sides of the + * comparison, and the flag is left alone instead of being wrongly released. + * + * Safe under optimistic concurrency: the write below still requires + * `version: expectedVersion`, so a body that changed between this read and + * the update fails as a version mismatch rather than acting on stale text. + */ + const storedBodyFlags = + update.body !== undefined + ? await Skill.findById(id) + .select('body') + .lean() + .then((doc) => + doc + ? extractBooleanFlagsFromBody((doc as unknown as { body?: string }).body) + : undefined, + ) + : undefined; const issues: ValidationIssue[] = []; if (update.name !== undefined) issues.push(...validateSkillName(update.name)); if (update.description !== undefined) @@ -1604,7 +1640,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, bodyScan)); + issues.push(...validateBodyDerivedColumns(update.frontmatter, bodyScan, storedBodyFlags)); /* 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 @@ -1631,33 +1667,6 @@ export function createSkillMethods( throw error; } - const Skill = mongoose.models.Skill as Model; - /** - * A body edit may only release an invocation-mode flag that the SKILL.md - * text itself was carrying, so the stored body is read to see what it - * declared before. Without this, a skill whose flags live only in the - * frontmatter bag — the legacy shape `backfillDerivedFromFrontmatter` - * serves, and what setting flags through the API alone produces — would - * have a restriction silently lifted by an edit that never mentioned it. - * It also makes the body reader's blind spots harmless: a key it cannot - * see is invisible on both sides of the comparison, so the flag is left - * alone instead of being wrongly released. - * - * Safe under optimistic concurrency: the write below still requires - * `version: expectedVersion`, so a body that changed between this read and - * the update fails as a version mismatch rather than acting on stale text. - */ - const storedBodyFlags = - update.body !== undefined && update.frontmatter === undefined - ? await Skill.findById(id) - .select('body') - .lean() - .then((doc) => - doc - ? extractBooleanFlagsFromBody((doc as unknown as { body?: string }).body) - : undefined, - ) - : undefined; const setPayload: Record = {}; const unsetPayload: Record = {}; if (update.name !== undefined) setPayload.name = update.name;