mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🧯 fix: Let Only the Body's Own Prior Declaration Release a Restriction
Self-review found the previous guard was the wrong shape. Gating on "the new body has a frontmatter block" still released a bag-only restriction whenever the edited body happened to carry a block, and any key the body reader cannot see (a quoted `"user-invocable":`) looked like a removal too. Both are instances of one class: treating the reader's silence as a declaration. A body edit may now only remove a flag that the STORED body declared, read back under the same version guard that protects the write. An edit can therefore release what the author wrote into the file, a skill whose flags were never in the text keeps them, and a key the reader misses is invisible on both sides of the comparison — so its blind spots degrade to no-ops instead of silent releases. The structured-bag contract is untouched: a bag that omits a key still removes it, which stays the escape hatch for flags the body never had. Two reader divergences fixed with it, both confirmed against the real modules: the body scanner now unquotes keys, so `"user-invocable": false` is honored the way the importer already honored it; and an empty flag value is a placeholder rather than a malformed boolean even when the line scan finds nothing, which an indented mapping or a quoted key can cause. A corpus of 26 frontmatter shapes now runs through both the import path and the inline-body path with identical columns in 25 — the residual is duplicate keys differing only in case, where the file is ambiguous by construction (js-yaml takes the last, the line reader the first) and neither answer can release a restriction.
This commit is contained in:
Danny Avila
parent
0c5cd2ed84
commit
1d139666ae
4 changed files with 157 additions and 80 deletions
|
|
@ -510,6 +510,31 @@ describe('parseFrontmatter', () => {
|
|||
},
|
||||
);
|
||||
|
||||
it('treats an empty flag value as a placeholder even when the mapping is indented', () => {
|
||||
/* The line scan is anchored at column zero, so an indented key yields no raw
|
||||
text to inspect. Judging by the parsed shape keeps a mid-edit placeholder
|
||||
from being reported as a malformed value. */
|
||||
const raw = `---\n name: n\n description: d\n user-invocable:\n---\n\nbody`;
|
||||
const result = parseFrontmatter(raw);
|
||||
|
||||
expect(result.userInvocable).toBeUndefined();
|
||||
expect(result.invalidBooleans).toEqual([]);
|
||||
});
|
||||
|
||||
it('still flags a malformed value when the mapping is indented', () => {
|
||||
const raw = `---\n name: n\n description: d\n user-invocable: tru\n---\n\nbody`;
|
||||
|
||||
expect(parseFrontmatter(raw).invalidBooleans).toEqual(['user-invocable']);
|
||||
});
|
||||
|
||||
it.each(['"user-invocable"', "'user-invocable'"])('reads a %s quoted key', (key) => {
|
||||
const raw = `---\nname: n\ndescription: d\n${key}: false\n---\n\nbody`;
|
||||
const result = parseFrontmatter(raw);
|
||||
|
||||
expect(result.userInvocable).toBe(false);
|
||||
expect(result.frontmatter).toEqual({ 'user-invocable': false });
|
||||
});
|
||||
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -100,6 +100,18 @@ function stripInlineComment(value: string): string {
|
|||
return value.trim();
|
||||
}
|
||||
|
||||
/** Strip one layer of matching YAML quotes, if present. */
|
||||
function unquoteScalar(value: string): string {
|
||||
if (
|
||||
value.length >= 2 &&
|
||||
((value[0] === '"' && value[value.length - 1] === '"') ||
|
||||
(value[0] === "'" && value[value.length - 1] === "'"))
|
||||
) {
|
||||
return value.slice(1, -1).trim();
|
||||
}
|
||||
return value;
|
||||
}
|
||||
|
||||
function normalizeFrontmatterKeys(frontmatter: Record<string, unknown>): Record<string, unknown> {
|
||||
return Object.entries(frontmatter).reduce<Record<string, unknown>>((acc, [key, value]) => {
|
||||
const normalizedKey = key.toLowerCase();
|
||||
|
|
@ -135,8 +147,17 @@ function parseBoolean(value: unknown, rawValue?: string): boolean | undefined {
|
|||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a flag line carries no value yet — `user-invocable:`, a
|
||||
* comment-only value, or empty quotes. All are mid-edit states rather than
|
||||
* malformed booleans, so they are treated as if the key were absent.
|
||||
*/
|
||||
function hasBooleanPlaceholder(rawValue?: string): boolean {
|
||||
return rawValue !== undefined && stripInlineComment(rawValue).length === 0;
|
||||
if (rawValue === undefined) {
|
||||
return false;
|
||||
}
|
||||
const stripped = stripInlineComment(rawValue);
|
||||
return stripped.length === 0 || unquoteScalar(stripped).length === 0;
|
||||
}
|
||||
|
||||
type ResolvedBooleanFlag = { value?: boolean; invalidKey?: string };
|
||||
|
|
@ -154,11 +175,18 @@ function readPresentBooleanFlag(
|
|||
key: string,
|
||||
): ResolvedBooleanFlag {
|
||||
const rawValue = getRawFrontmatterValue(block, key);
|
||||
const value = parseBoolean(getCaseInsensitive(frontmatter, key), rawValue);
|
||||
if (value === undefined && !hasBooleanPlaceholder(rawValue)) {
|
||||
return { invalidKey: key };
|
||||
const parsedValue = getCaseInsensitive(frontmatter, key);
|
||||
const value = parseBoolean(parsedValue, rawValue);
|
||||
if (value !== undefined) {
|
||||
return { value };
|
||||
}
|
||||
return { value };
|
||||
if (rawValue === undefined) {
|
||||
/* No line to cross-check — the key is quoted, or the mapping is indented.
|
||||
Judge by the parsed shape instead: an empty value is a placeholder, while
|
||||
anything else present is a value that failed to read as a boolean. */
|
||||
return parsedValue == null ? {} : { invalidKey: key };
|
||||
}
|
||||
return hasBooleanPlaceholder(rawValue) ? {} : { invalidKey: key };
|
||||
}
|
||||
|
||||
function resolveBooleanFlag(
|
||||
|
|
|
|||
|
|
@ -1000,7 +1000,13 @@ describe('Skill CRUD methods', () => {
|
|||
expect(viaName?.disableModelInvocation).toBeUndefined();
|
||||
});
|
||||
|
||||
it('keeps bag-only restrictions when a body edit declares no frontmatter block', async () => {
|
||||
it.each([
|
||||
['a body with no frontmatter block', 'Still a plain body, just edited.'],
|
||||
[
|
||||
'a body whose frontmatter block never mentioned the flags',
|
||||
'---\nname: bag-only-restricted\ndescription: A demo skill.\ncategory-ish: x\n---\n\nEdited.',
|
||||
],
|
||||
])('keeps bag-only restrictions across %s', async (_label, newBody) => {
|
||||
/* The legacy / API-only shape: flags live in the bag, and the body never
|
||||
declared them. A body edit there is not a statement about invocation
|
||||
channels, so it must not lift the restriction — silently opening a
|
||||
|
|
@ -1027,7 +1033,7 @@ describe('Skill CRUD methods', () => {
|
|||
const updated = await methods.updateSkill({
|
||||
id,
|
||||
expectedVersion: 1,
|
||||
update: { body: 'Still a plain body, just edited.' },
|
||||
update: { body: newBody },
|
||||
});
|
||||
expect(updated.status).toBe('updated');
|
||||
|
||||
|
|
@ -1036,12 +1042,12 @@ describe('Skill CRUD methods', () => {
|
|||
expect(reloaded?.disableModelInvocation).toBe(true);
|
||||
});
|
||||
|
||||
it('releases a bag-only restriction once the body declares a frontmatter block without it', async () => {
|
||||
/* The counterpart: an explicit block that omits the key IS a declaration,
|
||||
which is what makes the release path in the UI work. */
|
||||
it('releases a bag-only restriction when an explicit frontmatter bag drops it', async () => {
|
||||
/* Structured callers keep the long-standing contract: a bag that omits the
|
||||
key removes it. That is the escape hatch for flags the body never had. */
|
||||
const legacy = await Skill.create({
|
||||
name: 'bag-only-released',
|
||||
description: 'Flags set through the API, then declared away.',
|
||||
description: 'Flags set through the API, then dropped through the API.',
|
||||
body: 'Plain body, no frontmatter.',
|
||||
frontmatter: { 'user-invocable': false },
|
||||
author: owner._id,
|
||||
|
|
@ -1056,13 +1062,7 @@ describe('Skill CRUD methods', () => {
|
|||
);
|
||||
const id = (legacy._id as mongoose.Types.ObjectId).toString();
|
||||
|
||||
await methods.updateSkill({
|
||||
id,
|
||||
expectedVersion: 1,
|
||||
update: {
|
||||
body: '---\nname: bag-only-released\ndescription: A demo skill.\n---\n\nBody.',
|
||||
},
|
||||
});
|
||||
await methods.updateSkill({ id, expectedVersion: 1, update: { frontmatter: {} } });
|
||||
|
||||
const reloaded = await methods.getSkillByName('bag-only-released', [legacy._id]);
|
||||
expect(reloaded?.userInvocable).toBeUndefined();
|
||||
|
|
@ -1079,6 +1079,20 @@ describe('Skill CRUD methods', () => {
|
|||
expect(skill.userInvocable).toBe(false);
|
||||
});
|
||||
|
||||
it('reads a flag written with a quoted key', async () => {
|
||||
/* The importer honors a quoted key because js-yaml unquotes it; the body
|
||||
reader has to strip the quotes itself or the same file would behave
|
||||
differently depending on how it reached the server. */
|
||||
const { skill } = await methods.createSkill(
|
||||
makeSkillInput({
|
||||
name: 'quoted-key',
|
||||
body: '---\nname: quoted-key\ndescription: A demo skill.\n"user-invocable": 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({
|
||||
|
|
|
|||
|
|
@ -774,16 +774,6 @@ type BodyAlwaysApplyResult =
|
|||
/** Body-derived state for every boolean flag mirrored onto a column. */
|
||||
type BodyFlagResults = Record<SkillBooleanColumn, BodyAlwaysApplyResult>;
|
||||
|
||||
type BodyFlagScan = {
|
||||
/**
|
||||
* Whether the body carried a YAML frontmatter block at all. A body without
|
||||
* one declares nothing, so it must not be read as declaring the *absence* of
|
||||
* a restriction — see `updateSkill`.
|
||||
*/
|
||||
hasBlock: boolean;
|
||||
flags: BodyFlagResults;
|
||||
};
|
||||
|
||||
const BODY_FLAG_BY_KEY = new Map<string, SkillBooleanFlag>(
|
||||
SKILL_BOOLEAN_FLAGS.flatMap((flag) =>
|
||||
[flag.key, ...flag.aliases].map((key) => [key.toLowerCase(), flag] as const),
|
||||
|
|
@ -856,6 +846,18 @@ function readContinuedFlagValue(
|
|||
return null;
|
||||
}
|
||||
|
||||
/** Strip one layer of matching YAML quotes, if present. */
|
||||
function unquoteScalar(value: string): string {
|
||||
if (
|
||||
value.length >= 2 &&
|
||||
((value[0] === '"' && value[value.length - 1] === '"') ||
|
||||
(value[0] === "'" && value[value.length - 1] === "'"))
|
||||
) {
|
||||
return value.slice(1, -1).trim();
|
||||
}
|
||||
return value;
|
||||
}
|
||||
|
||||
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
|
||||
|
|
@ -864,13 +866,7 @@ function readBodyFlagValue(rawValue: string): BodyAlwaysApplyResult {
|
|||
if (value === '') {
|
||||
return { status: 'absent' };
|
||||
}
|
||||
if (
|
||||
value.length >= 2 &&
|
||||
((value[0] === '"' && value[value.length - 1] === '"') ||
|
||||
(value[0] === "'" && value[value.length - 1] === "'"))
|
||||
) {
|
||||
value = value.slice(1, -1).trim();
|
||||
}
|
||||
value = unquoteScalar(value);
|
||||
if (value === '') {
|
||||
return { status: 'absent' };
|
||||
}
|
||||
|
|
@ -910,7 +906,7 @@ function readBodyFlagValue(rawValue: string): BodyAlwaysApplyResult {
|
|||
* value YAML continues onto the following line is read from there rather than
|
||||
* being treated as an unwritten placeholder.
|
||||
*/
|
||||
function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagScan {
|
||||
function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagResults {
|
||||
const results: BodyFlagResults = {
|
||||
alwaysApply: { status: 'absent' },
|
||||
userInvocable: { status: 'absent' },
|
||||
|
|
@ -918,12 +914,12 @@ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagScan {
|
|||
};
|
||||
const block = extractBodyFrontmatterBlock(body);
|
||||
if (block === null) {
|
||||
return { hasBlock: false, flags: results };
|
||||
return results;
|
||||
}
|
||||
const lines = block.split('\n');
|
||||
const baseIndent = findMappingIndent(lines);
|
||||
if (baseIndent === null) {
|
||||
return { hasBlock: true, flags: results };
|
||||
return results;
|
||||
}
|
||||
const canonical = new Map<SkillBooleanColumn, BodyAlwaysApplyResult>();
|
||||
const aliased = new Map<SkillBooleanColumn, BodyAlwaysApplyResult>();
|
||||
|
|
@ -937,7 +933,7 @@ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagScan {
|
|||
if (colon === -1) {
|
||||
continue;
|
||||
}
|
||||
const key = trimmed.slice(0, colon).trim().toLowerCase();
|
||||
const key = unquoteScalar(trimmed.slice(0, colon).trim()).toLowerCase();
|
||||
const flag = BODY_FLAG_BY_KEY.get(key);
|
||||
if (!flag) {
|
||||
continue;
|
||||
|
|
@ -960,11 +956,11 @@ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagScan {
|
|||
results[flag.column] = resolved;
|
||||
}
|
||||
}
|
||||
return { hasBlock: true, flags: results };
|
||||
return results;
|
||||
}
|
||||
|
||||
function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyResult {
|
||||
return extractBooleanFlagsFromBody(body).flags.alwaysApply;
|
||||
return extractBooleanFlagsFromBody(body).alwaysApply;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1258,7 +1254,7 @@ export function createSkillMethods(
|
|||
and for the derivation cascades. Avoids parsing the same YAML
|
||||
frontmatter block twice per create. */
|
||||
const bodyScan = data.body !== undefined ? extractBooleanFlagsFromBody(data.body) : undefined;
|
||||
const bodyAlwaysApply = bodyScan?.flags.alwaysApply;
|
||||
const bodyAlwaysApply = bodyScan?.alwaysApply;
|
||||
const issues: ValidationIssue[] = [
|
||||
...validateSkillName(data.name),
|
||||
...validateSkillDescription(data.description),
|
||||
|
|
@ -1266,7 +1262,7 @@ export function createSkillMethods(
|
|||
...validateSkillDisplayTitle(data.displayTitle),
|
||||
...validateSkillFrontmatter(data.frontmatter),
|
||||
...validateAlwaysApply(data.alwaysApply),
|
||||
...validateBodyDerivedColumns(data.frontmatter, bodyScan?.flags),
|
||||
...validateBodyDerivedColumns(data.frontmatter, bodyScan),
|
||||
];
|
||||
/* Body-level `always-apply:` only needs to be well-formed when a
|
||||
higher-precedence source won't override it (see
|
||||
|
|
@ -1324,7 +1320,7 @@ export function createSkillMethods(
|
|||
*/
|
||||
const bodyDerived: { userInvocable?: boolean; disableModelInvocation?: boolean } = {};
|
||||
for (const column of BODY_DERIVED_COLUMNS) {
|
||||
const resolved = resolveBodyDerivedColumn(column, derived, bodyScan?.flags);
|
||||
const resolved = resolveBodyDerivedColumn(column, derived, bodyScan);
|
||||
if (resolved !== undefined) {
|
||||
bodyDerived[column] = resolved;
|
||||
}
|
||||
|
|
@ -1597,7 +1593,7 @@ export function createSkillMethods(
|
|||
Avoids parsing the same YAML frontmatter block twice per update. */
|
||||
const bodyScan =
|
||||
update.body !== undefined ? extractBooleanFlagsFromBody(update.body) : undefined;
|
||||
const bodyAlwaysApply = bodyScan?.flags.alwaysApply;
|
||||
const bodyAlwaysApply = bodyScan?.alwaysApply;
|
||||
const issues: ValidationIssue[] = [];
|
||||
if (update.name !== undefined) issues.push(...validateSkillName(update.name));
|
||||
if (update.description !== undefined)
|
||||
|
|
@ -1608,7 +1604,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?.flags));
|
||||
issues.push(...validateBodyDerivedColumns(update.frontmatter, bodyScan));
|
||||
/* 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
|
||||
|
|
@ -1636,6 +1632,32 @@ export function createSkillMethods(
|
|||
}
|
||||
|
||||
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;
|
||||
|
|
@ -1665,46 +1687,34 @@ export function createSkillMethods(
|
|||
/**
|
||||
* 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.
|
||||
* frontmatter (the only signal the UI edit flow sends).
|
||||
*
|
||||
* A body carrying no frontmatter block at all declares nothing and so does
|
||||
* not count: skills whose flags live only in the bag (the legacy shape
|
||||
* `backfillDerivedFromFrontmatter` exists for, and what a caller setting
|
||||
* flags through the API alone produces) would otherwise have a restriction
|
||||
* silently lifted by an unrelated body edit. Losing a restriction that way
|
||||
* is worse than keeping one a step longer, so it takes an explicit
|
||||
* frontmatter block — with the key removed from it — to release.
|
||||
* Removal is the delicate half. A bag that omits a key removes it, which is
|
||||
* the long-standing contract for callers sending structured frontmatter. A
|
||||
* body may only remove what that same body used to declare — compared
|
||||
* against `storedBodyFlags` — so an edit can release a restriction the
|
||||
* author wrote into the file, while a skill whose flags were never in the
|
||||
* text keeps them. When a body-driven removal happens, the bag's copy of
|
||||
* that key goes too, otherwise `backfillDerivedFromFrontmatter` would read
|
||||
* the restriction straight back over the unset column on the next lookup.
|
||||
*/
|
||||
const declaresColumns =
|
||||
update.frontmatter !== undefined ||
|
||||
(update.body !== undefined && bodyScan?.hasBlock === true);
|
||||
for (const column of BODY_DERIVED_COLUMNS) {
|
||||
const resolved = resolveBodyDerivedColumn(column, bagDerived, bodyScan?.flags);
|
||||
const resolved = resolveBodyDerivedColumn(column, bagDerived, bodyScan);
|
||||
if (resolved !== undefined) {
|
||||
setPayload[column] = resolved;
|
||||
} else if (declaresColumns) {
|
||||
unsetPayload[column] = '';
|
||||
continue;
|
||||
}
|
||||
}
|
||||
/**
|
||||
* 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 && bodyScan?.hasBlock === true) {
|
||||
for (const flag of SKILL_BOOLEAN_FLAGS) {
|
||||
for (const key of [flag.key, ...flag.aliases]) {
|
||||
unsetPayload[`frontmatter.${key}`] = '';
|
||||
}
|
||||
if (update.frontmatter !== undefined) {
|
||||
unsetPayload[column] = '';
|
||||
continue;
|
||||
}
|
||||
if (storedBodyFlags?.[column].status !== 'valid') {
|
||||
continue;
|
||||
}
|
||||
unsetPayload[column] = '';
|
||||
const flag = SKILL_BOOLEAN_FLAGS.find((candidate) => candidate.column === column);
|
||||
for (const key of flag ? [flag.key, ...flag.aliases] : []) {
|
||||
unsetPayload[`frontmatter.${key}`] = '';
|
||||
}
|
||||
}
|
||||
if (update.category !== undefined) setPayload.category = update.category;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue