🔓 fix: Don't Lock Skills Out of Saving Over Pre-Existing Malformed Flags

Three findings from the self-review's completeness pass.

A skill whose STORED SKILL.md already carried a malformed flag became unsavable.
`validateBodyDerivedColumns` ran on every body-carrying update, and the skill
editor resubmits the whole file on every save, so a body containing
`user-invocable: yes` rejected edits to unrelated fields — the user could not
even fix the description. Those documents exist precisely because pre-fix import
returned 201 for that value, and `yes`/`no`/`on`/`off` read as booleans in YAML
1.1, so it is an ordinary authoring shape. The stored-body scan is now taken
before validation and a flag that was already malformed no longer blocks the
save; a value this edit introduces is still rejected.

The end-to-end spec never ran in CI: `test:ci` ignores `\.*integration\.`, and
the named integration scripts are scoped to cache, s3 and agents, so the only
test asserting the issue's reproduction against a real `createSkill` was skipped
everywhere. Renamed to `import.db.spec.ts`, matching the several packages/api
specs that already boot MongoMemoryServer in the default run.

Parity between the two frontmatter readers was asserted only in a commit
message. `parity.db.spec.ts` now feeds 23 shapes through both the upload path and
the inline-body path and requires identical columns, with the two known
asymmetries pinned as their own cases: tab indentation, which only the YAML
parser rejects, and duplicate keys differing in case, which the file leaves
ambiguous. Neither can release a restriction.

Skill validation failures now carry `message`, as the import handler already did.
The client falls back to a generic string when it is absent, so a rejected flag
line was previously undiagnosable from the UI.

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<Columns> {
+  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<Columns> {
+  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 });
+  });
+});

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' });

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' }));
 

packages/data-schemas/src/methods/skill.ts

@@ -996,6 +996,7 @@ function resolveBodyDerivedColumn(
 function validateBodyDerivedColumns(
   frontmatter: Record<string, unknown> | 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<ISkillDocument>;
+    /**
+     * 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<ISkillDocument>;
-    /**
-     * 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<string, unknown> = {};
     const unsetPayload: Record<string, ''> = {};
     if (update.name !== undefined) setPayload.name = update.name;