diff --git a/.changeset/fix-skill-ir-validation.md b/.changeset/fix-skill-ir-validation.md new file mode 100644 index 000000000..e9757abd7 --- /dev/null +++ b/.changeset/fix-skill-ir-validation.md @@ -0,0 +1,5 @@ +--- +"agent-bundle": patch +--- + +Validate raw portable Skill metadata before building the sanitized Skill IR, and report AB3006 diagnostics for unknown fields nested under Claude, Cursor, and Codex targets. diff --git a/packages/agent-bundle/src/config/validate.ts b/packages/agent-bundle/src/config/validate.ts index bbae8e267..bb9da845c 100644 --- a/packages/agent-bundle/src/config/validate.ts +++ b/packages/agent-bundle/src/config/validate.ts @@ -831,18 +831,31 @@ const validateMcp = ( }; const validateSkill = (skill: SkillDocument): Diagnostic[] => { + const portableIssues = validateAgentSkillsFrontmatter({ + ...(Object.hasOwn(skill.frontmatter, 'allowed-tools') + ? { 'allowed-tools': skill.frontmatter['allowed-tools'] } + : {}), + ...(Object.hasOwn(skill.frontmatter, 'compatibility') + ? { compatibility: skill.frontmatter.compatibility } + : {}), + ...(Object.hasOwn(skill.frontmatter, 'description') + ? { description: skill.frontmatter.description } + : {}), + ...(Object.hasOwn(skill.frontmatter, 'license') + ? { license: skill.frontmatter.license } + : {}), + ...(Object.hasOwn(skill.frontmatter, 'metadata') + ? { metadata: skill.frontmatter.metadata } + : {}), + ...(Object.hasOwn(skill.frontmatter, 'name') + ? { name: skill.frontmatter.name } + : {}), + }); const ir = parseSkillIr(skill); const diagnostics = [...ir.diagnostics]; const name = ir.portable.name ?? skill.frontmatter.name; - diagnostics.push(...validateAgentSkillsFrontmatter({ - ...(ir.portable.allowedTools === undefined ? {} : { 'allowed-tools': ir.portable.allowedTools }), - ...(ir.portable.compatibility === undefined ? {} : { compatibility: ir.portable.compatibility }), - ...(ir.portable.description === undefined ? {} : { description: ir.portable.description }), - ...(ir.portable.license === undefined ? {} : { license: ir.portable.license }), - ...(ir.portable.metadata === undefined ? {} : { metadata: ir.portable.metadata }), - ...(ir.portable.name === undefined ? {} : { name: ir.portable.name }), - }).map((issue) => { + diagnostics.push(...portableIssues.map((issue) => { const location = issue.field ?? (issue.instancePath === '' ? 'root' : issue.instancePath); return sourceDiagnostic( issue.field === 'name' ? 'AB4002' : issue.field === 'description' ? 'AB4003' : 'AB4007', diff --git a/packages/agent-bundle/src/skills/parse-ir.ts b/packages/agent-bundle/src/skills/parse-ir.ts index 8beaf1e3d..b1d1a94a2 100644 --- a/packages/agent-bundle/src/skills/parse-ir.ts +++ b/packages/agent-bundle/src/skills/parse-ir.ts @@ -31,6 +31,40 @@ const claudeOnlyKeys = new Set([ const sharedKeys = new Set(['disable-model-invocation', 'paths']); const cursorOnlyKeys = new Set(['color', 'globs', 'icon']); const authoringKeys = new Set(['targets']); +const claudeTargetKeys = new Set([ + ...claudeOnlyKeys, + ...sharedKeys, + 'allowed-tools', + 'allowedTools', + 'argumentHint', + 'disableModelInvocation', + 'disallowedTools', + 'userInvocable', + 'whenToUse', +]); +const cursorTargetKeys = new Set([ + ...cursorOnlyKeys, + ...sharedKeys, + 'disableModelInvocation', +]); +const codexTargetKeys = new Set(['dependencies', 'interface', 'policy']); +const codexInterfaceKeys = new Set([ + 'brandColor', + 'brand_color', + 'defaultPrompt', + 'default_prompt', + 'displayName', + 'display_name', + 'iconLarge', + 'icon_large', + 'iconSmall', + 'icon_small', + 'shortDescription', + 'short_description', +]); +const codexPolicyKeys = new Set(['allowImplicitInvocation', 'allow_implicit_invocation']); +const codexDependenciesKeys = new Set(['tools']); +const codexToolKeys = new Set(['description', 'transport', 'type', 'url', 'value']); const isPlainRecord = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value); @@ -202,6 +236,18 @@ const unknownField = (source: string, field: string): Diagnostic => ({ sourcePath: source, }); +const reportUnknownFields = ( + record: Readonly>, + allowed: ReadonlySet, + prefix: string, + source: string, + diagnostics: Diagnostic[], +): void => { + for (const key of Object.keys(record)) { + if (!allowed.has(key)) diagnostics.push(unknownField(source, `${prefix}.${key}`)); + } +}; + const sidecarFromResource = (document: SkillDocument): SkillSidecarRef | undefined => { const resource = document.resources.find((entry) => entry.relativePath === 'agents/openai.yaml'); if (resource === undefined) return undefined; @@ -229,6 +275,55 @@ const peelTargets = ( } const unknown = Object.keys(value).filter((key) => key !== 'claude' && key !== 'codex' && key !== 'cursor'); for (const key of unknown) diagnostics.push(unknownField(source, `targets.${key}`)); + if (isPlainRecord(value.claude)) { + reportUnknownFields(value.claude, claudeTargetKeys, 'targets.claude', source, diagnostics); + } + if (isPlainRecord(value.cursor)) { + reportUnknownFields(value.cursor, cursorTargetKeys, 'targets.cursor', source, diagnostics); + } + if (isPlainRecord(value.codex)) { + reportUnknownFields(value.codex, codexTargetKeys, 'targets.codex', source, diagnostics); + if (isPlainRecord(value.codex.interface)) { + reportUnknownFields( + value.codex.interface, + codexInterfaceKeys, + 'targets.codex.interface', + source, + diagnostics, + ); + } + if (isPlainRecord(value.codex.policy)) { + reportUnknownFields( + value.codex.policy, + codexPolicyKeys, + 'targets.codex.policy', + source, + diagnostics, + ); + } + if (isPlainRecord(value.codex.dependencies)) { + reportUnknownFields( + value.codex.dependencies, + codexDependenciesKeys, + 'targets.codex.dependencies', + source, + diagnostics, + ); + if (Array.isArray(value.codex.dependencies.tools)) { + value.codex.dependencies.tools.forEach((tool, index) => { + if (isPlainRecord(tool)) { + reportUnknownFields( + tool, + codexToolKeys, + `targets.codex.dependencies.tools[${index}]`, + source, + diagnostics, + ); + } + }); + } + } + } const claude = isPlainRecord(value.claude) ? claudeFrom({ ...value.claude, diff --git a/packages/agent-bundle/tests/normalization.test.ts b/packages/agent-bundle/tests/normalization.test.ts index 3f79135c5..ba957d2b8 100644 --- a/packages/agent-bundle/tests/normalization.test.ts +++ b/packages/agent-bundle/tests/normalization.test.ts @@ -463,6 +463,25 @@ it('maps pinned Agent Skills schema issues to stable source diagnostics without ]); }); +it('validates raw optional portable Skill fields before sanitizing the Skill IR', () => { + const root = '/workspace/project'; + const document = skill(root, 'review-tools', 'review-tools'); + document.frontmatter.compatibility = false; + + expect(validateSource( + loadedProject({ plugin: { name: 'review-tools', version: '1.0.0' } }), + { skills: [document] }, + registry, + )).toEqual([ + { + code: 'AB4007', + message: 'Skill frontmatter compatibility must be string.', + severity: 'error', + sourcePath: document.source, + }, + ]); +}); + it('diagnoses a missing plugin object instead of throwing', () => { const loaded = loadedProject({} as AgentBundleConfig); diff --git a/packages/agent-bundle/tests/skill-ir.test.ts b/packages/agent-bundle/tests/skill-ir.test.ts index 70ef88b8b..99151b578 100644 --- a/packages/agent-bundle/tests/skill-ir.test.ts +++ b/packages/agent-bundle/tests/skill-ir.test.ts @@ -293,6 +293,62 @@ describe('canonical Skill IR', () => { })]); }); + it('rejects unknown nested fields for every typed host target without dropping valid fields', async () => { + const markdown = [ + '---', + 'name: review', + 'description: Review a change.', + 'targets:', + ' claude:', + ' model: sonnet', + ' display_nmae: Review change', + ' cursor:', + ' color: blue', + ' display_nmae: Review change', + ' codex:', + ' display_nmae: Review change', + ' interface:', + ' display_name: Review change', + ' display_nmae: Review change', + ' policy:', + ' allow_implicit_invocation: true', + ' allow_implcit_invocation: true', + ' dependencies:', + ' dependency_typo: true', + ' tools:', + ' - type: mcp', + ' value: review', + ' tool_typo: true', + '---', + '', + '# Review', + '', + ].join('\n'); + const root = await projectRoot({ 'skills/review/SKILL.md': markdown }); + const ir = parseSkillIr(await parseSkill(join(root, 'skills', 'review'), root)); + + expect(ir.diagnostics).toEqual([ + 'targets.claude.display_nmae', + 'targets.cursor.display_nmae', + 'targets.codex.display_nmae', + 'targets.codex.interface.display_nmae', + 'targets.codex.policy.allow_implcit_invocation', + 'targets.codex.dependencies.dependency_typo', + 'targets.codex.dependencies.tools[0].tool_typo', + ].map((field) => expect.objectContaining({ + code: 'AB3006', + message: expect.stringContaining(field), + severity: 'error', + }))); + expect(ir.extensions.claude).toEqual(expect.objectContaining({ model: 'sonnet' })); + expect(ir.extensions.cursor).toEqual(expect.objectContaining({ color: 'blue' })); + expect(ir.extensions.codex).toEqual(expect.objectContaining({ + dependencies: { tools: [expect.objectContaining({ type: 'mcp', value: 'review' })] }, + interface: { displayName: 'Review change' }, + policy: { allowImplicitInvocation: true }, + })); + }); + it('surfaces the shared-vs-per-host skills tree as an inspect-visible evidence decision', async () => { const root = await projectRoot({ 'skills/review/SKILL.md': [