Skill lifecycle phase 1: typed frontmatter, usage tracking, plumbing fixes - #621
Conversation
Plan for the self-improvement loop: outcome -> skill pump on a reflection branch, curation in cortex maintenance, one origin-scoped skill_manage tool, and provenance/usage tracking in per-agent SQLite. Studied against the Hermes agent implementation and a live 126-skill corpus. Supersedes skill-authoring.md. Five build phases, this doc is the spec for all of them.
Phase 1 of docs/design-docs/skill-lifecycle.md:
- Replace the hand-rolled frontmatter parser with serde_yaml into a typed
SkillFrontmatter (platforms hard-gate against the host OS, tags,
related_skills). Unknown fields and unknown platform values are tolerated.
- Support subdirs (references/templates/scripts/assets) surface as
linked_files from read_skill; hidden dirs excluded from discovery.
- New skill_usage table + SkillUsageStore: created_by provenance, lifecycle
state, read/patch counters. Rows seed on first sight, installer marks
'installed', read_skill bumps read_count and reactivates stale skills.
WriteOrigin defined for the phase-2 skill_manage rails.
- API skill mutations reload the live SkillSet deterministically instead of
relying on the file watcher; instance installs reload all agents.
- Watcher creates skills roots before watching and classifies changes by
prefix against watched roots instead of contains("skills").
- skills_search HTTP errors report the status instead of body.len().
- read_skill output capped at the 50KB tool budget; index descriptions
truncated to 80 chars.
- skills_search/install_skill ported to the channel toolset (cortex-chat
TODO); branches get the skill index fragment and read_skill.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis change defines the skill lifecycle, adds typed metadata and SQLite usage tracking, integrates skill reloads with runtime initialization and mutations, and exposes skill metadata and tools through branch and channel workflows. ChangesSkill lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/skills.rs`:
- Around line 190-195: Preserve effective skill provenance across all listed
sites in src/api/skills.rs:190-195, src/api/skills.rs:288-290,
src/api/skills.rs:337-345, and src/api/skills.rs:501-501. In the installation
flow, reconcile usage from the reloaded SkillSet and call record_installed only
for skills whose instance source is effective; do not mark workspace-overridden
skills as installed. During removal, retain and reconcile the usage row when an
instance or builtin fallback becomes effective. For direct API uploads, record
user provenance through the appropriate usage API instead of record_installed.
In `@src/skills.rs`:
- Around line 360-362: Update index_description to enforce DESCRIPTION_BUDGET by
Unicode character count rather than byte length, replacing
truncate_utf8_ellipsis with the appropriate character-aware truncation helper.
Add a Unicode test covering multibyte descriptions that remain within the
character budget.
- Around line 62-87: Update the platform matching logic that uses Platform::host
and the deserialized platform list so Platform::Other never satisfies the
contains check; compare only known variants (Linux, Macos, and Windows). Add a
test covering an unknown frontmatter platform on an unsupported host, asserting
the skill is not loaded.
- Around line 488-498: Update the linked-file traversal around the directory
read and entry iteration to ignore only NotFound errors for optional support
directories, while logging or propagating all other I/O failures. Replace the
silent `let Ok(...) else { continue }` and `while let Ok(...)` handling without
changing successful directory and file discovery behavior.
In `@src/skills/builtin.rs`:
- Around line 36-50: Update parse_builtin to apply the existing platform matcher
to frontmatter.platforms before constructing Skill, returning the same skip
behavior used for filesystem skills when the host is unsupported. Add a test
covering that a platform-specific built-in skill is excluded on a non-matching
host.
In `@src/skills/usage.rs`:
- Around line 111-115: Update the INSERT conflict-update clause in the skill
usage installation query to set origin_conversation_id to NULL alongside
created_by = 'installed', ensuring installation removes prior agent provenance.
Add a conflict test covering an existing agent-created row and verify its origin
conversation is cleared after installation.
In `@src/tools.rs`:
- Around line 782-783: The cleanup calls for SkillsSearchTool::NAME and
InstallSkillTool::NAME silently discard remove_tool failures; update this
removal flow to handle each Result by logging unexpected errors or propagating
them, while preserving the existing cleanup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d559efed-2563-4fbf-9160-a8231c66a416
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (20)
docs/design-docs/skill-lifecycle.mdmigrations/20260808000001_skill_usage.sqlprompts/en/branch.md.j2prompts/en/fragments/skills_branch.md.j2src/agent/channel_dispatch.rssrc/api/agents.rssrc/api/skills.rssrc/config/runtime.rssrc/config/watcher.rssrc/main.rssrc/prompts/engine.rssrc/prompts/text.rssrc/skills.rssrc/skills/builtin.rssrc/skills/installer.rssrc/skills/usage.rssrc/tools.rssrc/tools/install_skill.rssrc/tools/read_skill.rssrc/tools/skills_search.rs
- Platform::Other never matches the host: a skill gated to unrecognized platforms stays off every host, and an unrecognized host OS fails gates instead of passing them. Built-in skills now respect the same gate. - Description budget applied by character count, not bytes. - linked_files discovery logs I/O errors instead of discarding them (NotFound stays silent — support dirs are optional). - record_installed clears origin_conversation_id when replacing an agent-created row. - Uploaded skills seed as 'user' provenance instead of 'installed' — they're user-provided, not registry installs.
Phase 1 of the skill lifecycle design (
docs/design-docs/skill-lifecycle.md, included in this PR). This lays the foundations for the self-improvement loop — typed skill metadata, per-skill provenance/usage tracking, and a pile of fixes in the existing skill plumbing. The mutation tool, reflection branch, and curation land in later phases.Typed frontmatter
The hand-rolled frontmatter parser is gone. SKILL.md frontmatter now parses through serde_yaml into a typed struct, so YAML lists and multiline scalars actually work:
Unknown fields are ignored (skills from other ecosystems carry
version,author, etc.), and unknown platform values deserialize toPlatform::Other— the skill is skipped on this host rather than failing to parse. Skills gated to another platform don't load at all.Skill directories can now carry support subdirs (
references/,templates/,scripts/,assets/); their files are excluded from discovery and returned byread_skillaslinked_files. Hidden directories (.archive,.git, ...) are excluded from discovery.skill_usage table
New per-agent migration tracking provenance and usage per skill:
created_by('user' | 'agent' | 'installed'), lifecyclestate,pinned, read/patch counters, timestamps. Skills on disk with no row get seeded on first sight withcreated_at = now(a fresh install shouldn't look 90 days stale) andcreated_by = 'user'(the origin that protects them from future auto-curation). The registry installer marks its skills'installed';read_skillbumpsread_countand reactivates stale skills.WriteOriginis defined here too — Phase 2'sskill_managerails dispatch on it.Fixes in passing
SkillSetdeterministically instead of hoping the file watcher notices (reload_after_skill_change). Instance-level installs reload every running agent.contains("skills").skills_searchwas formatting the HTTP status slot withbody.len(). Now it's the status.read_skilloutput is capped at the standard 50 KB tool budget.skill_manage).Toolset changes
skills_searchandinstall_skillare now on the channel toolset, completing the port out of the deprecated cortex-chat server (tools.rs TODO).config_inspectis the last unique tool left there.read_skill, so a branch can consult procedure before delegating — and it's the substrate the Phase 3 reflection branch builds on.Testing
New unit tests for YAML list/multiline parsing, platform tolerance, description truncation, and the usage store (seeding idempotence, read-count/reactivation, installed-provenance override, row removal) — the store tests run the real migration against in-memory SQLite.