From 286758449dc828ca0584497696bb97eea8eb15b3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 4 Aug 2026 08:39:17 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Keep=20Skill=20Parser=20M?= =?UTF-8?q?ock-Safe=20and=20Read=20Continued=20Body=20Flags?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups on the invocation-mode work. CI: parse.ts built its key lookup at module scope from a `SKILL_BOOLEAN_FLAGS` value imported out of data-schemas. Suites that replace that module with a partial mock (agents/openai/service.spec.ts mocks it as `logger` alone) left the import undefined, so the map construction threw before any test ran and took six `api` suites plus one `@librechat/api` shard down with it. The table is declared locally again — this module is pure text parsing and must load without the DB package initialized — and parse.test.ts asserts it still matches data-schemas. Codex P1: a body-only edit unset the derived column but left the stored frontmatter bag's copy in place, so `backfillDerivedFromFrontmatter` read the restriction back on the next `getSkillByName` and the release undid itself. A body-driven update now clears the bag's flag keys, handing authority to the columns; the SKILL.md body still carries the declarations. Codex P2 / Copilot: the body scanner treated `user-invocable:` with its value on the following line as an unwritten placeholder, and skipping every indented line also blinded it to a frontmatter block indented as a whole. It now reads keys at the mapping's own indentation and follows a lone indented scalar as a continuation value, matching what the import/sync parser already accepted. --- .../api/src/skills/__tests__/parse.test.ts | 15 +++ packages/api/src/skills/parse.ts | 16 +++- .../data-schemas/src/methods/skill.spec.ts | 72 ++++++++++++++ packages/data-schemas/src/methods/skill.ts | 94 +++++++++++++++++-- 4 files changed, 188 insertions(+), 9 deletions(-) create mode 100644 packages/api/src/skills/__tests__/parse.test.ts diff --git a/packages/api/src/skills/__tests__/parse.test.ts b/packages/api/src/skills/__tests__/parse.test.ts new file mode 100644 index 0000000000..8dd6b216c9 --- /dev/null +++ b/packages/api/src/skills/__tests__/parse.test.ts @@ -0,0 +1,15 @@ +import { SKILL_BOOLEAN_FLAGS as SCHEMA_BOOLEAN_FLAGS } from '@librechat/data-schemas'; +import { SKILL_BOOLEAN_FLAGS } from '../parse'; + +/** + * The parser keeps its own copy of the flag table so it stays loadable when a + * suite replaces `@librechat/data-schemas` with a partial mock. This test is + * what keeps that copy honest: the parser decides which keys are read out of a + * SKILL.md, data-schemas decides which columns they feed, and a mismatch would + * silently drop a flag on one side only. + */ +describe('SKILL_BOOLEAN_FLAGS', () => { + it('matches the table in data-schemas exactly', () => { + expect(SKILL_BOOLEAN_FLAGS).toEqual(SCHEMA_BOOLEAN_FLAGS); + }); +}); diff --git a/packages/api/src/skills/parse.ts b/packages/api/src/skills/parse.ts index 2b1b40580d..599ccdf71b 100644 --- a/packages/api/src/skills/parse.ts +++ b/packages/api/src/skills/parse.ts @@ -1,7 +1,21 @@ import yaml from 'js-yaml'; -import { SKILL_BOOLEAN_FLAGS } from '@librechat/data-schemas'; import type { SkillBooleanFlag, SkillBooleanColumn } from '@librechat/data-schemas'; +/** + * Boolean frontmatter flags mirrored onto first-class skill columns, declared + * locally on purpose. This module is pure text parsing and must stay loadable + * without `@librechat/data-schemas` initialized: several suites replace that + * module with a partial mock, and reading a value export from it at module + * scope resolves to `undefined` and throws before any test runs. The type is + * still imported (erased at compile time), and `parse.test.ts` asserts this + * table matches `SKILL_BOOLEAN_FLAGS` in data-schemas so the two cannot drift. + */ +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: [] }, +]; + export type ParsedSkillMarkdown = { name: string; description: string; diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 8f8db8be35..bd1f44c637 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -968,6 +968,78 @@ describe('Skill CRUD methods', () => { expect(updated.skill.disableModelInvocation).toBeUndefined(); }); + it('releasing a restriction survives a lookup that backfills from frontmatter', async () => { + /* An imported skill carries the flags in BOTH its body and its stored + bag. A body-only edit unsets the column, but if the bag kept its copy + `backfillDerivedFromFrontmatter` would read the restriction back on the + next lookup and the release would silently undo itself. */ + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'release-survives', + body: bodyWithFlags, + frontmatter: { 'user-invocable': false, 'disable-model-invocation': true }, + }), + ); + expect(skill.userInvocable).toBe(false); + + await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + body: '---\nname: release-survives\ndescription: A demo skill.\n---\n\nBody.', + }, + }); + + const viaId = await methods.getSkillById(skill._id); + expect(viaId?.frontmatter).not.toHaveProperty('user-invocable'); + expect(viaId?.frontmatter).not.toHaveProperty('disable-model-invocation'); + + /* getSkillByName runs the backfill; the restriction must stay released. */ + const viaName = await methods.getSkillByName('release-survives', [skill._id]); + expect(viaName?.userInvocable).toBeUndefined(); + expect(viaName?.disableModelInvocation).toBeUndefined(); + }); + + it('reads a flag whose YAML value continues on the next line', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'continued-value', + body: '---\nname: continued-value\ndescription: A demo skill.\nuser-invocable:\n false\n---\n\nBody.', + frontmatter: undefined, + }), + ); + expect(skill.userInvocable).toBe(false); + }); + + it('reads flags from a frontmatter block that is indented as a whole', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'indented-block', + body: '---\n name: indented-block\n description: A demo skill.\n disable-model-invocation: true\n---\n\nBody.', + frontmatter: undefined, + }), + ); + expect(skill.disableModelInvocation).toBe(true); + }); + + it('does not unset a column when the body still declares it on a continued line', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'continued-keeps', body: bodyWithFlags, frontmatter: undefined }), + ); + expect(skill.userInvocable).toBe(false); + + const updated = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + body: '---\nname: continued-keeps\ndescription: A demo skill.\nuser-invocable:\n false\n---\n\nEdited.', + }, + }); + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + expect(updated.skill.userInvocable).toBe(false); + }); + 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 }), diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 700b910744..b65ec6f6e8 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -797,6 +797,55 @@ function extractBodyFrontmatterBlock(body: string | undefined): string | null { return after.slice(0, closingIdx); } +function indentWidth(line: string): number { + let width = 0; + while (width < line.length && (line[width] === ' ' || line[width] === '\t')) { + width++; + } + return width; +} + +/** + * Indentation of the frontmatter mapping's own keys, taken from its first + * content line. Usually zero, but a block whose every key is indented is still + * valid YAML and its keys are still top-level. + */ +function findMappingIndent(lines: string[]): number | null { + for (const line of lines) { + const trimmed = line.trim(); + if (trimmed.length === 0 || trimmed.startsWith('#')) { + continue; + } + return indentWidth(line); + } + return null; +} + +/** + * Read a flag value that YAML continues on a following line, as in + * `user-invocable:` followed by an indented `false`. Only a lone indented + * scalar qualifies — anything carrying its own `:` is a nested mapping, which + * makes the flag a mapping rather than a boolean. + */ +function readContinuedFlagValue( + lines: string[], + keyIndex: number, + baseIndent: number, +): BodyAlwaysApplyResult | null { + for (let i = keyIndex + 1; i < lines.length; i++) { + const line = lines[i]; + const trimmed = line.trim(); + if (trimmed.length === 0 || trimmed.startsWith('#')) { + continue; + } + if (indentWidth(line) <= baseIndent || trimmed.includes(':')) { + return null; + } + return readBodyFlagValue(trimmed); + } + return null; +} + 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 @@ -844,9 +893,12 @@ function readBodyFlagValue(rawValue: string): BodyAlwaysApplyResult { * 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. + * consulted when the canonical key never appears. Only keys at the mapping's + * own indentation are read, so a nested mapping reusing a flag name + * (`metadata:` → ` user-invocable:`) is not mistaken for a declaration, while + * a frontmatter block that is indented as a whole still parses. A flag whose + * value YAML continues onto the following line is read from there rather than + * being treated as an unwritten placeholder. */ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagResults { const results: BodyFlagResults = { @@ -858,22 +910,32 @@ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagResults if (block === null) { return results; } + const lines = block.split('\n'); + const baseIndent = findMappingIndent(lines); + if (baseIndent === 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') { + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const trimmed = line.trim(); + if (trimmed.length === 0 || trimmed.startsWith('#') || indentWidth(line) !== baseIndent) { continue; } - const colon = line.indexOf(':'); + const colon = trimmed.indexOf(':'); if (colon === -1) { continue; } - const key = line.slice(0, colon).trim().toLowerCase(); + const key = trimmed.slice(0, colon).trim().toLowerCase(); const flag = BODY_FLAG_BY_KEY.get(key); if (!flag) { continue; } - const result = readBodyFlagValue(line.slice(colon + 1)); + let result = readBodyFlagValue(trimmed.slice(colon + 1)); + if (result.status === 'absent') { + result = readContinuedFlagValue(lines, i, baseIndent) ?? result; + } if (key === flag.key) { if (!canonical.has(flag.column)) { canonical.set(flag.column, result); @@ -1609,6 +1671,22 @@ export function createSkillMethods( unsetPayload[column] = ''; } } + /** + * A body-only edit rewrites the SKILL.md text without sending a bag, so the + * stored bag would keep flag keys the edited text no longer declares — and + * `backfillDerivedFromFrontmatter` reads those back over an unset column on + * the next lookup, resurrecting a restriction the author just removed. + * Hand flag authority to the columns resolved above by clearing the bag's + * copies; the SKILL.md body still carries the declarations, so nothing is + * lost, and the next save that does send a bag repopulates them. + */ + if (update.frontmatter === undefined && bodyFlags) { + for (const flag of SKILL_BOOLEAN_FLAGS) { + for (const key of [flag.key, ...flag.aliases]) { + unsetPayload[`frontmatter.${key}`] = ''; + } + } + } if (update.category !== undefined) setPayload.category = update.category; /** * Keep the indexed `alwaysApply` column in sync with whatever the update