From 0c5cd2ed84c9742ec2da9480e637aaa76f9c779e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 4 Aug 2026 08:56:30 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Require=20a=20Fr?= =?UTF-8?q?ontmatter=20Block=20Before=20a=20Body=20Edit=20Releases=20a=20R?= =?UTF-8?q?estriction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review caught a data-loss regression in the body cascade. A skill whose invocation flags live only in the frontmatter bag — the pre-Phase-6 shape `backfillDerivedFromFrontmatter` exists for, and what a caller setting flags through the API alone produces, since the bag need not be repeated in the SKILL.md text — had its restriction silently lifted by any body edit. Verified against the real methods: a `disable-model-invocation: true` skill came back `undefined` on both columns after an unrelated body rewrite, quietly exposing it to the model. The body now only counts as declaring these flags when it actually carries a YAML frontmatter block. A block that omits the key is still a declaration, so the release path this PR added keeps working for imported skills; a body with no block at all declares nothing and leaves the columns and the bag alone. Losing a restriction silently is worse than keeping one an edit longer. `alwaysApply` keeps its existing "no frontmatter block means opt out" contract — it is opt-in and defaults to false, so absence there cannot lose a restriction. --- .../data-schemas/src/methods/skill.spec.ts | 68 +++++++++++++++++++ packages/data-schemas/src/methods/skill.ts | 50 ++++++++++---- 2 files changed, 103 insertions(+), 15 deletions(-) diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index bd1f44c637..a9f5c460ee 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -1000,6 +1000,74 @@ describe('Skill CRUD methods', () => { expect(viaName?.disableModelInvocation).toBeUndefined(); }); + it('keeps bag-only restrictions when a body edit declares no frontmatter block', async () => { + /* 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 + model-disabled skill is far worse than releasing one edit later. */ + const legacy = await Skill.create({ + name: 'bag-only-restricted', + description: 'Flags set through the API, never written into the body.', + body: 'Plain body, no frontmatter.', + frontmatter: { 'user-invocable': false, 'disable-model-invocation': true }, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + }); + /* `Skill.create` applies the schema defaults, so strip the columns to get + the real pre-Phase-6 shape: flags in the bag, columns absent. */ + await Skill.collection.updateOne( + { _id: legacy._id }, + { $unset: { disableModelInvocation: '', userInvocable: '' } }, + ); + const id = (legacy._id as mongoose.Types.ObjectId).toString(); + + const updated = await methods.updateSkill({ + id, + expectedVersion: 1, + update: { body: 'Still a plain body, just edited.' }, + }); + expect(updated.status).toBe('updated'); + + const reloaded = await methods.getSkillByName('bag-only-restricted', [legacy._id]); + expect(reloaded?.userInvocable).toBe(false); + 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. */ + const legacy = await Skill.create({ + name: 'bag-only-released', + description: 'Flags set through the API, then declared away.', + body: 'Plain body, no frontmatter.', + frontmatter: { 'user-invocable': false }, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + }); + await Skill.collection.updateOne( + { _id: legacy._id }, + { $unset: { disableModelInvocation: '', userInvocable: '' } }, + ); + 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.', + }, + }); + + const reloaded = await methods.getSkillByName('bag-only-released', [legacy._id]); + expect(reloaded?.userInvocable).toBeUndefined(); + }); + it('reads a flag whose YAML value continues on the next line', async () => { const { skill } = await methods.createSkill( makeSkillInput({ diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index b65ec6f6e8..67ce4ef1be 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -774,6 +774,16 @@ type BodyAlwaysApplyResult = /** Body-derived state for every boolean flag mirrored onto a column. */ type BodyFlagResults = Record; +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( SKILL_BOOLEAN_FLAGS.flatMap((flag) => [flag.key, ...flag.aliases].map((key) => [key.toLowerCase(), flag] as const), @@ -900,7 +910,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): BodyFlagResults { +function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagScan { const results: BodyFlagResults = { alwaysApply: { status: 'absent' }, userInvocable: { status: 'absent' }, @@ -908,12 +918,12 @@ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagResults }; const block = extractBodyFrontmatterBlock(body); if (block === null) { - return results; + return { hasBlock: false, flags: results }; } const lines = block.split('\n'); const baseIndent = findMappingIndent(lines); if (baseIndent === null) { - return results; + return { hasBlock: true, flags: results }; } const canonical = new Map(); const aliased = new Map(); @@ -950,11 +960,11 @@ function extractBooleanFlagsFromBody(body: string | undefined): BodyFlagResults results[flag.column] = resolved; } } - return results; + return { hasBlock: true, flags: results }; } function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyResult { - return extractBooleanFlagsFromBody(body).alwaysApply; + return extractBooleanFlagsFromBody(body).flags.alwaysApply; } /** @@ -1247,8 +1257,8 @@ export function createSkillMethods( /* 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 bodyScan = data.body !== undefined ? extractBooleanFlagsFromBody(data.body) : undefined; + const bodyAlwaysApply = bodyScan?.flags.alwaysApply; const issues: ValidationIssue[] = [ ...validateSkillName(data.name), ...validateSkillDescription(data.description), @@ -1256,7 +1266,7 @@ export function createSkillMethods( ...validateSkillDisplayTitle(data.displayTitle), ...validateSkillFrontmatter(data.frontmatter), ...validateAlwaysApply(data.alwaysApply), - ...validateBodyDerivedColumns(data.frontmatter, bodyFlags), + ...validateBodyDerivedColumns(data.frontmatter, bodyScan?.flags), ]; /* Body-level `always-apply:` only needs to be well-formed when a higher-precedence source won't override it (see @@ -1314,7 +1324,7 @@ export function createSkillMethods( */ const bodyDerived: { userInvocable?: boolean; disableModelInvocation?: boolean } = {}; for (const column of BODY_DERIVED_COLUMNS) { - const resolved = resolveBodyDerivedColumn(column, derived, bodyFlags); + const resolved = resolveBodyDerivedColumn(column, derived, bodyScan?.flags); if (resolved !== undefined) { bodyDerived[column] = resolved; } @@ -1585,9 +1595,9 @@ export function createSkillMethods( /* 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 = + const bodyScan = update.body !== undefined ? extractBooleanFlagsFromBody(update.body) : undefined; - const bodyAlwaysApply = bodyFlags?.alwaysApply; + const bodyAlwaysApply = bodyScan?.flags.alwaysApply; const issues: ValidationIssue[] = []; if (update.name !== undefined) issues.push(...validateSkillName(update.name)); if (update.description !== undefined) @@ -1598,7 +1608,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)); + issues.push(...validateBodyDerivedColumns(update.frontmatter, bodyScan?.flags)); /* 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 @@ -1661,10 +1671,20 @@ export function createSkillMethods( * `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. + * + * 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. */ - const declaresColumns = update.frontmatter !== undefined || update.body !== undefined; + const declaresColumns = + update.frontmatter !== undefined || + (update.body !== undefined && bodyScan?.hasBlock === true); for (const column of BODY_DERIVED_COLUMNS) { - const resolved = resolveBodyDerivedColumn(column, bagDerived, bodyFlags); + const resolved = resolveBodyDerivedColumn(column, bagDerived, bodyScan?.flags); if (resolved !== undefined) { setPayload[column] = resolved; } else if (declaresColumns) { @@ -1680,7 +1700,7 @@ export function createSkillMethods( * 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) { + 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}`] = '';