diff --git a/Cargo.lock b/Cargo.lock index ef4e572ab..da1cb021e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8150,6 +8150,19 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "serde_yaml" +version = "0.9.34+deprecated" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +dependencies = [ + "indexmap 2.13.0", + "itoa", + "ryu", + "serde", + "unsafe-libyaml", +] + [[package]] name = "serenity" version = "0.12.5" @@ -8503,6 +8516,7 @@ dependencies = [ "semver", "serde", "serde_json", + "serde_yaml", "serenity", "sha2", "slack-morphism", @@ -10091,6 +10105,12 @@ dependencies = [ "subtle", ] +[[package]] +name = "unsafe-libyaml" +version = "0.2.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" + [[package]] name = "untrusted" version = "0.9.0" diff --git a/Cargo.toml b/Cargo.toml index f6b6a0195..65887c00f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ thiserror = "2.0" # Serialization serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +serde_yaml = "0.9" # LLM / Rig framework rig = { version = "0.33", package = "rig-core", features = ["derive"] } diff --git a/docs/design-docs/skill-lifecycle.md b/docs/design-docs/skill-lifecycle.md new file mode 100644 index 000000000..fb029319b --- /dev/null +++ b/docs/design-docs/skill-lifecycle.md @@ -0,0 +1,364 @@ +# Skill Lifecycle: Self-Improvement for Spacebot + +Supersedes `skill-authoring.md`. That doc proposed the first slice (a `write_skill` +tool plus a reflection branch); this one covers the full loop it was reaching for: +**outcome → candidate procedure → authored or patched skill → provenance and usage +tracking → curation and pruning.** The reference implementation studied is +Hermes (`nousresearch/hermes-agent` at 2026-08-08 HEAD), which runs this loop in +production, plus a 126-skill live corpus from a heavily-used Hermes instance. + +## Why skills and not more memory + +Memory answers "who is the user and what is going on." Skills answer "how do we +do this class of task here." Hermes draws this line explicitly in its review +prompts, and it's the right line: a correction like "stop posting walls of text +in Discord" is not a fact about the user — it's a standing procedure change, and +it belongs in the skill governing that task class. Spacebot has real memory +(cortex, LanceDB) and a real skill loader, but nothing that turns experience +into skills. That's the gap. + +## What exists today (audited 2026-08-08) + +- `SkillSet` with three sources, precedence Builtin < Instance < Workspace + (`src/skills.rs:66-103`). Loaded via `ArcSwap` on `RuntimeConfig`, hot-reloaded + by the file watcher. +- Prompt injection is index-only (name + description): channels get + `fragments/skills_channel.md.j2`, workers get `skills_worker.md.j2` with + suggested flags. **Branches get no skill index at all** (`branch.md.j2:8`). +- Tools: `read_skill` (workers + Direct-mode channels), `skills_search` and + `install_skill` **only in the deprecated cortex-chat toolset** + (`src/tools.rs:1068-1075` has the TODO to port them). +- No create, no patch, no delete tool. `SkillSet::remove` exists but is only + reachable over HTTP/CLI. No provenance beyond `source_repo` (repo name only). + No usage tracking. `CreateSkill.tsx` is a stub. +- Known defects to fix in passing: hand-rolled frontmatter parser drops YAML + lists and multiline scalars (`src/skills.rs:353-410`); API skill mutations + never call `reload_skills` and rely on the watcher (`src/api/skills.rs:258`); + the watcher only watches skill dirs that existed at startup + (`src/config/watcher.rs:93-110`); `skills_search` formats the HTTP status + slot with `body.len()` (`src/tools/skills_search.rs:215-219`); `read_skill` + output is unbounded. + +## What Hermes proves works + +Distilled from source, not docs. The load-bearing rules, with where they live +in Hermes: + +1. **Progressive disclosure with a paid-for index.** Name + ≤60-char description + in every system prompt; full body only via explicit load. The budget is + enforced at *create* time, not truncated at read time + (`skill_manager_tool.py:604-617`). +2. **Provenance is ambient, never model-supplied.** A context-bound write origin + (foreground vs. autonomous review) decides `created_by`; the model cannot + claim authorship semantics because it never passes the field + (`skill_provenance.py`). +3. **Only agent-created skills are auto-curated.** A skill created by the user, + installed from a registry, or authored in a foreground conversation is + outside curator jurisdiction unless the user explicitly adopts it into + management. Provenance is a user declaration. +4. **Autonomy narrows the blast radius.** Same verb, different semantics by + origin: foreground delete removes; autonomous delete archives (recoverable). + Pin blocks deletion for a present user but blocks *all writes* for an + autonomous actor — consent is the axis, not the operation. +5. **Read-before-write for autonomous editors.** The review fork must have + loaded the exact target file in the current pass before it may patch it — + a mechanical rail against patching imagined content + (`skill_manager_tool.py:424`). +6. **Fail closed on unverified destruction.** Consolidation deletes require a + declared `absorbed_into` target that exists on disk. This fixed a real + incident where an LLM consolidation pass archived active skills with zero + verified merges (`skill_manager_tool.py:463`). +7. **Deterministic maintenance and LLM consolidation are separate passes.** + Stale/archive transitions are pure code over usage counters, always on. + The opinionated LLM rewrite pass is off by default. +8. **The review prompt is the policy surface**, and its sharpest rules are + negative: never persist environment-dependent failures, negative tool + claims ("browser tools don't work" hardens into a refusal cited for months), + transient errors, or one-off task narratives. Prefer patching an existing + umbrella skill over creating a new one; new-skill names must describe the + task class, not the incident. +9. **Telemetry in a sidecar, not frontmatter** — usage data never creates merge + pressure on content. (Spacebot does one better: per-agent SQLite.) +10. **Demote, never hide.** When the index needs compacting, drop descriptions + but keep every name visible — models don't rediscover what vanishes. + +Corpus evidence backs the shape: 126 skills, directory-per-skill with support +files, categories with `DESCRIPTION.md`, `related_skills` cross-links, and +skills genuinely evolving (one skill edited four times in four days with +version bumps). + +## Architecture mapping + +Hermes fakes process separation with forked Python agents. Spacebot already has +the real thing, so each loop lands on the process built for it: + +| Loop | Hermes | Spacebot | +|---|---|---| +| Capability index + explicit load | system prompt + `skill_view` | existing index fragments + `read_skill` (unchanged shape) | +| Outcome → skill pump | post-turn forked agent replaying the conversation | **reflection branch** — branches already clone channel history at spawn; this is exactly the replay-fork, natively | +| Slow curation | idle-triggered curator fork | **cortex maintenance** — cortex already owns background cognition and has a maintenance cadence | +| Mutation surface | `skill_manage` tool | new `skill_manage` tool, origin-scoped | +| Usage/provenance store | JSON sidecar with file locks | table in the agent's SQLite | +| Backups | tar.gz snapshots | tar.gz snapshots (same; skills dirs aren't git repos) | + +## Design + +### 1. Format and parsing + +Keep directory-per-skill `SKILL.md`. Replace the hand-rolled frontmatter parser +with `serde_yaml` into a typed struct: + +```rust +#[derive(Deserialize)] +struct SkillFrontmatter { + name: Option, // falls back to directory name + description: String, + platforms: Option>, // hard gate vs. host OS; absent = all + tags: Option>, + related_skills: Option>, // advisory, surfaced by read_skill + source_repo: Option, // kept for installer compatibility +} +``` + +Unknown fields ignored (Hermes corpus carries `version`, `author`, `license` — +decorative there too; tolerate, don't require). Support subdirectories inside a +skill: `references/`, `templates/`, `scripts/`, `assets/` — excluded from skill +discovery, listed by `read_skill` as `linked_files` so a skill body can point +at deeper material without inflating the index. Keep `{baseDir}` substitution. +No inline shell expansion — Hermes ships it off by default and it's a prompt +injection surface we don't need. + +Description budget: 80 chars, enforced on create/edit through `skill_manage` +and the write API only — pre-existing and installed skills render truncated +with an ellipsis rather than failing to load. + +### 2. Provenance and usage: `skill_usage` table + +Per-agent SQLite (new migration — schema is append-only per repo policy): + +```sql +CREATE TABLE skill_usage ( + skill_name TEXT PRIMARY KEY, -- lowercased canonical name + created_by TEXT NOT NULL, -- 'user' | 'agent' | 'installed' + origin_conversation_id TEXT, -- set when created_by = 'agent' + state TEXT NOT NULL DEFAULT 'active', -- 'active'|'stale'|'archived' + pinned INTEGER NOT NULL DEFAULT 0, + read_count INTEGER NOT NULL DEFAULT 0, + patch_count INTEGER NOT NULL DEFAULT 0, + last_read_at TEXT, + last_patched_at TEXT, + created_at TEXT NOT NULL, + archived_at TEXT +); +``` + +`read_skill` bumps `read_count`; `skill_manage` bumps `patch_count`; the +installer inserts `created_by = 'installed'`. Skills present on disk with no +row get one seeded on first sight with `created_at = now` — a newly noticed +skill's staleness clock starts now, not at epoch (Hermes does this; it prevents +mass-archiving a fresh install). + +`WriteOrigin` rides the tool deps, set by the process constructing the tool +server — `User` for channel/API/CLI paths, `Agent` for the reflection branch +and cortex curation. The model never supplies it. + +### 3. `skill_manage` tool + +One tool, action-dispatched, mirroring the shape that works in Hermes: + +``` +skill_manage(action, name, ...) + create { content, category? } -- full SKILL.md text + patch { old_string, new_string, replace_all? } + edit { content } -- full rewrite + delete { absorbed_into? } + write_file { file_path, file_content } -- under references|templates|scripts|assets + remove_file { file_path } +``` + +Writes always target the agent's workspace skills dir (autonomous writes never +touch instance-level or builtin skills). Validation, all origins: + +- name: `^[a-z0-9][a-z0-9._-]*$`, ≤64 chars; category a single path segment. +- frontmatter parses, has description; body non-empty; description ≤80 on + create/edit. +- size caps: SKILL.md 100 KB, support files 1 MiB. +- path rails: reject `..` before allow-listing, canonicalize and verify the + resolved path stays inside the skill dir, refuse targets reached via symlink. +- delete refuses builtin (already true), anything outside a skills root, and + a skills root itself. + +Origin-scoped rails (the Hermes rules, ported as code not prompt): + +- `WriteOrigin::Agent` may not modify installed (`created_by = 'installed'`), + pinned, or instance-level skills. +- `WriteOrigin::Agent` must have `read_skill`'d the exact target this session + before patch/edit (tracked in the tool server's session state). +- `WriteOrigin::Agent` delete → archive: move the directory to + `{workspace}/skills/.archive/{name}/`, set `state = 'archived'`. `User` + delete removes the directory. `.archive` is excluded from discovery. +- delete with `absorbed_into` requires the named skill to exist on disk and + differ from the target. +- Pin: blocks delete for `User`, blocks every mutation for `Agent`. + +Every successful mutation calls `reload_skills` directly (the deterministic +path the `install_skill` tool already uses) — no reliance on the watcher. + +Tool placement: branches and cortex get `skill_manage`; workers keep +`read_skill` only (workers execute tasks, they don't legislate procedure); +channels don't get it — a channel that wants to save a procedure spawns the +reflection branch with focus text, which keeps the conversational loop clean +and gives every skill write the same restricted, auditable surface. While in +here, complete the `src/tools.rs:1068` TODO: `skills_search`/`install_skill` +move to the channel toolset and the deprecated cortex-chat server drops. + +### 4. Reflection branch — the pump + +A silent branch spawned after work that likely produced a reusable lesson. +Branches already clone channel history and system prompt at spawn, which is +precisely Hermes's replay-fork, minus the Python. + +**Trigger** (channel-side, after the turn's response is delivered): +tool iterations this turn ≥ `reflection_min_tool_iterations` (default 10), +or a worker attached to the conversation finished with `Success`/`Partial` +after ≥ that many iterations. Gated by `reflection_cooldown_secs` (default +3600) per conversation, and skipped entirely for cron-originated turns. +Counter-based, not idle-based: the signal that something was learned is that +real work happened, not that the user went quiet. (This replaces +skill-authoring.md's idle/turn-count gates.) + +**Constraints on the branch:** +- toolset: `skill_manage`, `read_skill`, `skills_list` (new: name/desc/state + listing backed by the usage table), plus the memory-save tools. Nothing else + — no shell, no messaging, no spawn. +- it cannot message the user; its outcome is a one-line summary logged and + surfaced as a low-priority status event (interface can render "learned: + patched *discord-rendering*" the way Hermes prints its 💾 line). +- `WriteOrigin::Agent`, so every rail in §3 applies mechanically. +- capped at 6 LLM turns (matches the prior doc's budget). + +**Prompt policy** (new `prompts/en/reflection.md.j2`; the prompt *is* the +product here, port Hermes's semantics not its text): +- decide first whether anything is worth keeping; ending with no writes is + acceptable, but treat a session where the user corrected the agent's + procedure as a strong write signal. +- preference ladder: patch the skill that was loaded this session → patch an + existing related skill → add a `references/` file to one → only then create + a new skill, named for the task class, never the incident. +- user frustration and corrections are skill signals, not just memory signals. +- the negative-capture list, verbatim in spirit: no environment-dependent + failures, no negative capability claims about tools, no transient errors + that resolved, no one-off narratives, no unresolved failure logs dressed as + procedure. + +### 5. Curation — cortex maintenance + +Two passes, run from cortex's existing maintenance cadence (weekly default, +config-gated), only over skills with `created_by = 'agent'` or explicitly +adopted ones: + +**Deterministic (always on when curation is enabled):** pure code over the +usage table. `active → stale` after `stale_after_days` (default 30) without a +read; `stale → archived` after `archive_after_days` (default 90); any read +reactivates. Skips pinned skills and any skill referenced by a cron job or +routine. Before any pass that will mutate, snapshot the workspace skills tree +(tar.gz + manifest under `{workspace}/skills/.snapshots/`, retention 5), and +write a run report row so the interface can show what happened. + +**Consolidation (off by default):** an LLM pass in a cortex-spawned worker +with the same restricted toolset, allowed to merge overlapping skills — every +delete requires `absorbed_into`, enforced by §3's rail, so it can only archive +what it demonstrably merged. This stays off until the deterministic pass has +proven boring. + +**User controls** (CLI + API, interface later): `pin`/`unpin`, `adopt` (flip +`created_by` to `'agent'` — the explicit act of handing a skill to curation), +`archive`/`restore`, `snapshots`/`rollback`. Rollback snapshots before +restoring, so it's undoable. + +### 6. Surfaces + +- `POST /agents/skills/write` — backs `CreateSkill.tsx` (currently a stub); + same validation as `skill_manage(create)` with `WriteOrigin::User`. While + in the API: make all mutating skill endpoints call `reload_skills` + deterministically, fixing the existing reload gap. +- `SkillInspector` gains the usage row: created-by, state, counts, pin toggle, + origin conversation link when agent-created. +- Watcher fixes ride along: watch skills roots even when created after + startup (create-then-watch), and replace the `contains("skills")` substring + classification with prefix matching against the actual watched roots. +- `read_skill`: apply the standard 50 KB tool-output cap, return + `linked_files` and `related_skills`. +- Branches get the skill index fragment they currently lack. + +### 7. Config + +```toml +[skills] +write_approval = false # stage agent writes for user approval instead of committing + +[skills.reflection] +enabled = true +min_tool_iterations = 10 +cooldown_secs = 3600 +max_turns = 6 + +[skills.curation] +enabled = true +interval_days = 7 +stale_after_days = 30 +archive_after_days = 90 +consolidation = false +snapshot_retention = 5 +``` + +All hot-reloadable via the existing `RuntimeConfig` pattern. `write_approval` +staging (pending records + approve/reject over API, diff rendering in the +interface) is designed in but built last — Hermes ships it off by default and +the origin rails carry the real safety load. + +## Phases + +**Phase 1 — foundations.** serde_yaml frontmatter with the typed struct and +new fields; support-subdir handling + `linked_files`; `skill_usage` migration +and read-count plumbing; `WriteOrigin` on tool deps; fix the API reload gap, +watcher gaps, `skills_search` status-format bug; cap `read_skill`; port +`skills_search`/`install_skill` out of the deprecated cortex-chat toolset; +give branches the skill index. + +**Phase 2 — mutation.** `skill_manage` with the full validation and +origin-rail set; archive semantics; `reload_skills` on every mutation; +`skills_list` tool; CLI parity (`spacebot skill pin|adopt|archive|restore`). + +**Phase 3 — the pump.** Reflection branch: trigger plumbing in the channel +turn finalizer and worker-outcome path, restricted tool server, reflection +prompt, cooldown state, status-event surfacing. + +**Phase 4 — curation.** Deterministic pass in cortex maintenance with +snapshots, run reports, and rollback; pin/adopt/cron-reference protections; +consolidation pass behind its default-off flag. + +**Phase 5 — surfaces.** `POST /agents/skills/write` + CreateSkill UI; usage +and provenance in SkillInspector; reflection/curation activity in the +interface; `write_approval` staging mode; user docs. + +Each phase lands as its own PR and is independently shippable; Phases 3 and 4 +are prompt-heavy and should expect iteration after real transcripts. + +## Non-goals and deliberate divergences from Hermes + +- **No inline shell expansion in skill bodies** — injection surface, off by + default even in Hermes. +- **No skill bundles, org overlay, or trust-matrix hub** for now. The existing + skills.sh search + GitHub installer stay as-is; a content-hash install lock + can come with a future registry pass. +- **Dry-run, if added, is capability-level** (a tool server that refuses + mutations), not prompt-advisory. Hermes's prompt-banner dry-run is its one + rail we should not copy. +- **No static security scanner for agent-created skills.** Spacebot skills are + markdown injected into prompts; scripts they reference execute through the + existing sandboxed shell tools, which is where that enforcement belongs. +- **No semantic versioning or content history** — `patch_count` + + curation snapshots + (optional) user git on the skills dir cover it, same + posture as the prior doc. +- **No cross-agent skill sharing** yet; workspace scoping stands. diff --git a/migrations/20260808000001_skill_usage.sql b/migrations/20260808000001_skill_usage.sql new file mode 100644 index 000000000..bb83b8090 --- /dev/null +++ b/migrations/20260808000001_skill_usage.sql @@ -0,0 +1,20 @@ +-- Per-skill provenance and usage tracking. +-- +-- Skills on disk with no row get one seeded on first sight with +-- created_at = now, so a newly noticed skill's staleness clock starts at +-- discovery rather than at epoch. Only skills with created_by = 'agent' +-- are ever auto-curated; 'user' and 'installed' skills are outside curator +-- jurisdiction unless explicitly adopted. +CREATE TABLE skill_usage ( + skill_name TEXT PRIMARY KEY, -- lowercased canonical name + created_by TEXT NOT NULL, -- 'user' | 'agent' | 'installed' + origin_conversation_id TEXT, -- set when created_by = 'agent' + state TEXT NOT NULL DEFAULT 'active', -- 'active' | 'stale' | 'archived' + pinned INTEGER NOT NULL DEFAULT 0, + read_count INTEGER NOT NULL DEFAULT 0, + patch_count INTEGER NOT NULL DEFAULT 0, + last_read_at TEXT, + last_patched_at TEXT, + created_at TEXT NOT NULL, + archived_at TEXT +); diff --git a/prompts/en/branch.md.j2 b/prompts/en/branch.md.j2 index 524e0f50f..ad6dd75ba 100644 --- a/prompts/en/branch.md.j2 +++ b/prompts/en/branch.md.j2 @@ -39,6 +39,9 @@ Forget a memory by ID. Use this when the user wants something removed, or when y ### spacebot_docs Read embedded Spacebot docs, including `AGENTS.md`, `CHANGELOG.md`, and product docs from `docs/content/`. Use `action: "list"` to discover IDs, then `action: "read"` for the specific document. +### read_skill +Load the full instructions for a skill listed in ``. Read a skill before reasoning about work it covers, and pass skill names to spawned workers as `suggested_skills` rather than inlining their content. + ### spawn_worker If the user wants something done now and it needs execution tools (shell, file), spawn a worker. Give it a specific task description with enough context to work independently. The worker won't have the conversation history — it only knows what you tell it. If the user is describing something for later rather than requesting immediate action, save a **todo** memory instead. diff --git a/prompts/en/fragments/skills_branch.md.j2 b/prompts/en/fragments/skills_branch.md.j2 new file mode 100644 index 000000000..009a4e96d --- /dev/null +++ b/prompts/en/fragments/skills_branch.md.j2 @@ -0,0 +1,14 @@ +## Available Skills + +These skills are procedures this agent knows. When one is relevant to your reasoning, call `read_skill` to load its full instructions. + +When you spawn a worker for a task that matches a skill, pass the skill names as `suggested_skills` instead of inlining the skill's content into the task description — the worker reads the skills it needs itself. + + +{%- for skill in skills %} + + {{ skill.name }} + {{ skill.description }} + +{%- endfor %} + diff --git a/src/agent/channel_dispatch.rs b/src/agent/channel_dispatch.rs index 83295ca76..9270837b2 100644 --- a/src/agent/channel_dispatch.rs +++ b/src/agent/channel_dispatch.rs @@ -176,6 +176,14 @@ pub async fn spawn_branch_from_state( &rc.workspace_dir.display().to_string(), wiki_enabled, ) + .and_then(|prompt| { + let skills_prompt = rc.skills.load().render_branch_skills(&prompt_engine)?; + Ok(if skills_prompt.is_empty() { + prompt + } else { + format!("{prompt}\n\n{skills_prompt}") + }) + }) .and_then(|prompt| { prompt_engine.maybe_append_tool_use_enforcement( prompt, diff --git a/src/api/agents.rs b/src/api/agents.rs index 3ccc530df..f70b6a0e4 100644 --- a/src/api/agents.rs +++ b/src/api/agents.rs @@ -905,6 +905,20 @@ pub async fn create_agent_internal( skills, )); runtime_config.set_settings(settings_store.clone()); + let skill_usage_store = + std::sync::Arc::new(crate::skills::SkillUsageStore::new(db.sqlite.clone())); + runtime_config.set_skill_usage(skill_usage_store.clone()); + { + let skill_names: Vec = runtime_config + .skills + .load() + .iter() + .map(|s| s.name.to_lowercase()) + .collect(); + if let Err(error) = skill_usage_store.seed(&skill_names).await { + tracing::warn!(%error, "failed to seed skill usage rows"); + } + } let llm_manager = { let guard = state.llm_manager.read().await; diff --git a/src/api/skills.rs b/src/api/skills.rs index 1216755b5..51153506d 100644 --- a/src/api/skills.rs +++ b/src/api/skills.rs @@ -166,6 +166,36 @@ pub(super) struct RegistrySkillContentResponse { content: Option, } +/// Deterministically reload skills into live runtime configs after a skill +/// mutation, rather than relying on the file watcher, and record installed +/// provenance for any newly installed skills. +/// +/// `agent_id = None` reloads every running agent (instance-level change). +async fn reload_after_skill_change(state: &ApiState, agent_id: Option<&str>, installed: &[String]) { + let configs = state.runtime_configs.load(); + let instance_skills_dir = state.instance_dir.load().join("skills"); + + for (id, runtime_config) in configs.iter() { + if let Some(target) = agent_id + && target != id + { + continue; + } + + let workspace_skills_dir = runtime_config.workspace_dir.join("skills"); + let skills = + crate::skills::SkillSet::load(&instance_skills_dir, &workspace_skills_dir).await; + runtime_config.reload_skills(skills); + + if !installed.is_empty() + && let Some(store) = runtime_config.skill_usage.load().as_ref() + && let Err(error) = store.record_installed(installed).await + { + tracing::warn!(%error, agent_id = %id, "failed to record installed skills"); + } + } +} + /// List installed skills for an agent. #[utoipa::path( get, @@ -255,6 +285,9 @@ pub(super) async fn install_skill( StatusCode::INTERNAL_SERVER_ERROR })?; + let reload_target = (!req.instance).then_some(req.agent_id.as_str()); + reload_after_skill_change(&state, reload_target, &installed).await; + state.send_event(ApiEvent::ConfigReloaded); Ok(Json(InstallSkillResponse { installed })) @@ -301,6 +334,17 @@ pub(super) async fn remove_skill( } })?; + if removed_path.is_some() { + reload_after_skill_change(&state, Some(&req.agent_id), &[]).await; + + if let Some(runtime_config) = state.runtime_configs.load().get(&req.agent_id) + && let Some(store) = runtime_config.skill_usage.load().as_ref() + && let Err(error) = store.remove(&req.name).await + { + tracing::warn!(%error, skill = %req.name, "failed to remove skill usage row"); + } + } + state.send_event(ApiEvent::ConfigReloaded); tracing::info!( @@ -454,6 +498,9 @@ pub(super) async fn upload_skill( } if !all_installed.is_empty() { + // Uploads are user-provided, not registry installs — seeding during + // reload records them with 'user' provenance. + reload_after_skill_change(&state, Some(&query.agent_id), &[]).await; state.send_event(ApiEvent::ConfigReloaded); } diff --git a/src/config/runtime.rs b/src/config/runtime.rs index 8df809491..d9313f6e4 100644 --- a/src/config/runtime.rs +++ b/src/config/runtime.rs @@ -76,6 +76,8 @@ pub struct RuntimeConfig { pub cron_scheduler: ArcSwap>>, /// Settings store for agent-specific configuration. pub settings: ArcSwap>>, + /// Skill provenance and usage tracking, set after agent initialization. + pub skill_usage: ArcSwap>>, /// Prompt snapshot store for debugging prompt construction. pub prompt_snapshots: ArcSwap>>, /// Secrets store for encrypted credential storage. @@ -155,6 +157,7 @@ impl RuntimeConfig { cron_store: ArcSwap::from_pointee(None), cron_scheduler: ArcSwap::from_pointee(None), settings: ArcSwap::from_pointee(None), + skill_usage: ArcSwap::from_pointee(None), prompt_snapshots: ArcSwap::from_pointee(None), secrets: ArcSwap::from_pointee(None), sandbox: Arc::new(ArcSwap::from_pointee(agent_config.sandbox.clone())), @@ -186,6 +189,11 @@ impl RuntimeConfig { self.settings.store(Arc::new(Some(settings))); } + /// Set the skill usage store after initialization. + pub fn set_skill_usage(&self, store: Arc) { + self.skill_usage.store(Arc::new(Some(store))); + } + /// Set the secrets store after initialization. pub fn set_secrets(&self, secrets: Arc) { self.secrets.store(Arc::new(Some(secrets))); @@ -322,9 +330,24 @@ impl RuntimeConfig { } /// Reload skills from disk. + /// + /// Seeds usage rows for skills seen for the first time, so their + /// staleness clock starts at discovery. pub fn reload_skills(&self, skills: crate::skills::SkillSet) { + let names: Vec = skills.iter().map(|s| s.name.to_lowercase()).collect(); self.skills.store(Arc::new(skills)); tracing::info!("skills reloaded"); + + if let Some(store) = self.skill_usage.load().as_ref() + && let Ok(handle) = tokio::runtime::Handle::try_current() + { + let store = store.clone(); + handle.spawn(async move { + if let Err(error) = store.seed(&names).await { + tracing::warn!(%error, "failed to seed skill usage rows"); + } + }); + } } } diff --git a/src/config/watcher.rs b/src/config/watcher.rs index fa99a26fd..66b2717b2 100644 --- a/src/config/watcher.rs +++ b/src/config/watcher.rs @@ -90,25 +90,26 @@ pub fn spawn_file_watcher( tracing::warn!(%error, path = %config_path.display(), "failed to watch config file"); } - // Watch instance-level skills directory - let instance_skills_dir = instance_dir.join("skills"); - if instance_skills_dir.is_dir() - && let Err(error) = watcher.watch(&instance_skills_dir, RecursiveMode::Recursive) - { - tracing::warn!(%error, path = %instance_skills_dir.display(), "failed to watch instance skills dir"); + // Watch skills directories. Roots are created before watching so a + // dir that doesn't exist yet at startup is still covered, and kept + // for prefix-matching changed paths against actual skills roots. + let mut skill_roots: Vec = Vec::new(); + skill_roots.push(instance_dir.join("skills")); + for (_, workspace, _, _, _) in &agents { + skill_roots.push(workspace.join("skills")); + } + for root in &skill_roots { + if let Err(error) = std::fs::create_dir_all(root) { + tracing::warn!(%error, path = %root.display(), "failed to create skills dir"); + continue; + } + if let Err(error) = watcher.watch(root, RecursiveMode::Recursive) { + tracing::warn!(%error, path = %root.display(), "failed to watch skills dir"); + } } // Watch per-agent directories - for (_, workspace, identity_dir, _, _) in &agents { - // Watch workspace/skills for skill file changes - { - let path = workspace.join("skills"); - if path.is_dir() - && let Err(error) = watcher.watch(&path, RecursiveMode::Recursive) - { - tracing::warn!(%error, path = %path.display(), "failed to watch agent skills dir"); - } - } + for (_, _, identity_dir, _, _) in &agents { // Watch the agent root (identity_dir) for SOUL.md/IDENTITY.md/ROLE.md changes. // Identity files live outside the workspace, in the agent root directory. if let Err(error) = watcher.watch(identity_dir, RecursiveMode::NonRecursive) { @@ -157,7 +158,7 @@ pub fn spawn_file_watcher( }); let skills_changed = changed_paths .iter() - .any(|p| p.to_string_lossy().contains("skills")); + .any(|p| skill_roots.iter().any(|root| p.starts_with(root))); // Skip entirely if nothing relevant changed if !config_changed && !identity_changed && !skills_changed { diff --git a/src/main.rs b/src/main.rs index 4cd975f39..567295207 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2957,6 +2957,19 @@ async fn initialize_agents( )); runtime_config.set_settings(settings_store.clone()); + let skill_usage_store = Arc::new(spacebot::skills::SkillUsageStore::new(db.sqlite.clone())); + runtime_config.set_skill_usage(skill_usage_store.clone()); + { + let skill_names: Vec = runtime_config + .skills + .load() + .iter() + .map(|s| s.name.to_lowercase()) + .collect(); + if let Err(error) = skill_usage_store.seed(&skill_names).await { + tracing::warn!(%error, agent = %agent_config.id, "failed to seed skill usage rows"); + } + } runtime_config .prompt_snapshots .store(Arc::new(prompt_snapshot_store.clone())); diff --git a/src/prompts/engine.rs b/src/prompts/engine.rs index 72e3786f5..d178c5408 100644 --- a/src/prompts/engine.rs +++ b/src/prompts/engine.rs @@ -110,6 +110,10 @@ impl PromptEngine { "fragments/skills_worker", crate::prompts::text::get("fragments/skills_worker"), )?; + env.add_template( + "fragments/skills_branch", + crate::prompts::text::get("fragments/skills_branch"), + )?; env.add_template( "fragments/available_channels", crate::prompts::text::get("fragments/available_channels"), @@ -296,6 +300,19 @@ impl PromptEngine { ) } + /// Render the skills listing for a branch system prompt. + /// + /// Branches read skills directly via `read_skill` or pass names to + /// spawned workers as `suggested_skills`. + pub fn render_skills_branch(&self, skills: Vec) -> Result { + self.render( + "fragments/skills_branch", + context! { + skills => skills, + }, + ) + } + /// Render the worker system prompt with filesystem context and optional tool /// secret names. #[allow(clippy::too_many_arguments)] diff --git a/src/prompts/text.rs b/src/prompts/text.rs index d4400dc9d..56762342d 100644 --- a/src/prompts/text.rs +++ b/src/prompts/text.rs @@ -93,6 +93,9 @@ fn lookup(lang: &str, key: &str) -> &'static str { ("en", "fragments/skills_worker") => { include_str!("../../prompts/en/fragments/skills_worker.md.j2") } + ("en", "fragments/skills_branch") => { + include_str!("../../prompts/en/fragments/skills_branch.md.j2") + } ("en", "fragments/available_channels") => { include_str!("../../prompts/en/fragments/available_channels.md.j2") } diff --git a/src/skills.rs b/src/skills.rs index 424c1d6e2..34e68b37b 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -15,13 +15,89 @@ pub mod builtin; mod installer; +mod usage; pub use installer::{install_from_file, install_from_github}; +pub use usage::{SkillUsageRecord, SkillUsageStore, WriteOrigin}; use anyhow::Context as _; +use serde::Deserialize; use std::collections::HashMap; use std::path::{Path, PathBuf}; +/// Support subdirectories inside a skill directory. Files under these are +/// excluded from skill discovery and surfaced as `linked_files` by read_skill, +/// so a skill body can point at deeper material without inflating the index. +pub const SUPPORT_SUBDIRS: &[&str] = &["references", "templates", "scripts", "assets"]; + +/// Character budget for skill descriptions in prompt indexes. Enforced on +/// create/edit paths; pre-existing skills render truncated instead of +/// failing to load. +pub const DESCRIPTION_BUDGET: usize = 80; + +/// Typed SKILL.md frontmatter. Unknown fields are ignored so skills from +/// other ecosystems (carrying `version`, `author`, `license`, ...) load fine. +#[derive(Debug, Clone, Default, Deserialize)] +pub struct SkillFrontmatter { + /// Skill name; falls back to the directory name when absent. + pub name: Option, + /// Short description shown in prompt indexes. + pub description: Option, + /// Host platforms this skill applies to. Absent means all platforms. + pub platforms: Option>, + /// Free-form tags. + pub tags: Option>, + /// Names of related skills, surfaced by read_skill as navigation hints. + pub related_skills: Option>, + /// GitHub `owner/repo` this skill was installed from, if any. + pub source_repo: Option, +} + +/// A host platform a skill can be gated to. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Platform { + Linux, + Macos, + Windows, + /// Any value we don't recognize. Never matches the host, so a skill + /// gated to an unknown platform is skipped rather than failing to parse. + Other, +} + +impl<'de> Deserialize<'de> for Platform { + fn deserialize>(deserializer: D) -> Result { + let value = String::deserialize(deserializer)?; + Ok(match value.trim().to_ascii_lowercase().as_str() { + "linux" => Platform::Linux, + "macos" | "darwin" | "mac" => Platform::Macos, + "windows" | "win" => Platform::Windows, + _ => Platform::Other, + }) + } +} + +impl Platform { + /// The platform of the host this process is running on. + pub fn host() -> Platform { + match std::env::consts::OS { + "linux" => Platform::Linux, + "macos" => Platform::Macos, + "windows" => Platform::Windows, + _ => Platform::Other, + } + } + + /// Whether a skill's platform list matches the host. + /// + /// `Other` never matches — a skill gated to unrecognized platforms stays + /// off every host, and an unrecognized host OS fails all gates rather + /// than passing them. + pub fn list_matches_host(platforms: &[Platform]) -> bool { + let host = Platform::host(); + host != Platform::Other && platforms.contains(&host) + } +} + /// A loaded skill definition. #[derive(Debug, Clone)] pub struct Skill { @@ -39,6 +115,12 @@ pub struct Skill { pub source: SkillSource, /// GitHub `owner/repo` that this skill was installed from, if any. pub source_repo: Option, + /// Free-form tags from frontmatter. + pub tags: Vec, + /// Names of related skills from frontmatter, advisory. + pub related_skills: Vec, + /// Files under the skill's support subdirectories, relative to `base_dir`. + pub linked_files: Vec, } /// Where a skill was loaded from, used for precedence tracking. @@ -141,7 +223,7 @@ impl SkillSet { .into_iter() .map(|s| crate::prompts::SkillInfo { name: s.name.clone(), - description: s.description.clone(), + description: index_description(&s.description), location: s.file_path.display().to_string(), suggested: false, }) @@ -150,6 +232,34 @@ impl SkillSet { prompt_engine.render_skills_channel(skill_infos) } + /// Render the skills listing for injection into a branch system prompt. + /// + /// Branches see the same index as channels but read skills directly via + /// `read_skill`, or pass names to workers they spawn as `suggested_skills`. + pub fn render_branch_skills( + &self, + prompt_engine: &crate::prompts::PromptEngine, + ) -> crate::error::Result { + if self.skills.is_empty() { + return Ok(String::new()); + } + + let mut sorted_skills: Vec<&Skill> = self.skills.values().collect(); + sorted_skills.sort_by(|a, b| a.name.cmp(&b.name)); + + let skill_infos: Vec = sorted_skills + .into_iter() + .map(|s| crate::prompts::SkillInfo { + name: s.name.clone(), + description: index_description(&s.description), + location: s.file_path.display().to_string(), + suggested: false, + }) + .collect(); + + prompt_engine.render_skills_branch(skill_infos) + } + /// Render the skills listing for injection into a worker system prompt. /// /// Workers see all available skills with any channel-suggested skills flagged. @@ -173,7 +283,7 @@ impl SkillSet { .map(|s| crate::prompts::SkillInfo { suggested: suggested_lower.contains(&s.name.to_lowercase()), name: s.name.clone(), - description: s.description.clone(), + description: index_description(&s.description), location: s.file_path.display().to_string(), }) .collect(); @@ -253,6 +363,23 @@ impl SkillSet { } } +/// Truncate a description to the index budget (in characters) with an +/// ellipsis. +/// +/// The budget is enforced on create/edit paths; skills that predate it (or +/// were installed from a registry) render truncated rather than failing. +fn index_description(description: &str) -> String { + if description.chars().count() <= DESCRIPTION_BUDGET { + return description.to_string(); + } + + let truncated: String = description + .chars() + .take(DESCRIPTION_BUDGET.saturating_sub(3)) + .collect(); + format!("{truncated}...") +} + /// Public skill information for API responses. #[derive(Debug, Clone)] pub struct SkillInfo { @@ -267,6 +394,8 @@ pub struct SkillInfo { /// Load all skills from a directory. /// /// Each subdirectory containing a `SKILL.md` file is treated as a skill. +/// Hidden directories (`.archive`, `.snapshots`, `.git`, ...) are excluded +/// from discovery. async fn load_skills_from_dir(dir: &Path, source: SkillSource) -> anyhow::Result> { let mut skills = Vec::new(); @@ -280,13 +409,17 @@ async fn load_skills_from_dir(dir: &Path, source: SkillSource) -> anyhow::Result continue; } + if entry.file_name().to_string_lossy().starts_with('.') { + continue; + } + let skill_file = path.join("SKILL.md"); if !skill_file.exists() { continue; } match load_skill(&skill_file, &path, source.clone()).await { - Ok(skill) => { + Ok(Some(skill)) => { tracing::debug!( name = %skill.name, path = %skill_file.display(), @@ -294,6 +427,12 @@ async fn load_skills_from_dir(dir: &Path, source: SkillSource) -> anyhow::Result ); skills.push(skill); } + Ok(None) => { + tracing::debug!( + path = %skill_file.display(), + "skill gated to another platform, skipping" + ); + } Err(error) => { tracing::warn!( path = %skill_file.display(), @@ -308,18 +447,27 @@ async fn load_skills_from_dir(dir: &Path, source: SkillSource) -> anyhow::Result } /// Load a single skill from its SKILL.md file. +/// +/// Returns `Ok(None)` when the skill declares `platforms` that don't include +/// the host platform. async fn load_skill( file_path: &Path, base_dir: &Path, source: SkillSource, -) -> anyhow::Result { +) -> anyhow::Result> { let raw = tokio::fs::read_to_string(file_path) .await .with_context(|| format!("failed to read {}", file_path.display()))?; - let (frontmatter, body) = parse_frontmatter(&raw)?; + let (frontmatter, body) = parse_skill_markdown(&raw)?; - let name = frontmatter.get("name").cloned().unwrap_or_else(|| { + if let Some(platforms) = &frontmatter.platforms + && !Platform::list_matches_host(platforms) + { + return Ok(None); + } + + let name = frontmatter.name.clone().unwrap_or_else(|| { // Fall back to directory name if no name in frontmatter base_dir .file_name() @@ -328,36 +476,77 @@ async fn load_skill( .to_string() }); - let description = frontmatter.get("description").cloned().unwrap_or_default(); - let source_repo = frontmatter.get("source_repo").cloned(); - // Resolve {baseDir} template variable in the body let base_dir_str = base_dir.to_string_lossy(); let content = body.replace("{baseDir}", &base_dir_str); - Ok(Skill { + let linked_files = collect_linked_files(base_dir).await; + + Ok(Some(Skill { name, - description, + description: frontmatter.description.unwrap_or_default(), file_path: file_path.to_path_buf(), base_dir: base_dir.to_path_buf(), content, source, - source_repo, - }) + source_repo: frontmatter.source_repo, + tags: frontmatter.tags.unwrap_or_default(), + related_skills: frontmatter.related_skills.unwrap_or_default(), + linked_files, + })) } -/// Parse YAML frontmatter from a markdown file. +/// List files under the skill's support subdirectories, relative to `base_dir`. +async fn collect_linked_files(base_dir: &Path) -> Vec { + let mut files = Vec::new(); + + for subdir in SUPPORT_SUBDIRS { + let mut pending = vec![base_dir.join(subdir)]; + + while let Some(dir) = pending.pop() { + let mut entries = match tokio::fs::read_dir(&dir).await { + Ok(entries) => entries, + // Support directories are optional; absence is the common case. + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + tracing::warn!(%error, path = %dir.display(), "failed to read skill support dir"); + continue; + } + }; + loop { + match entries.next_entry().await { + Ok(Some(entry)) => { + let path = entry.path(); + if path.is_dir() { + pending.push(path); + } else if let Ok(relative) = path.strip_prefix(base_dir) { + files.push(relative.to_string_lossy().to_string()); + } + } + Ok(None) => break, + Err(error) => { + tracing::warn!(%error, path = %dir.display(), "failed to enumerate skill support dir"); + break; + } + } + } + } + } + + files.sort(); + files +} + +/// Split YAML frontmatter from a markdown file and parse it into +/// [`SkillFrontmatter`]. /// -/// Expects `---` delimiters. Returns the frontmatter key-value pairs and -/// the remaining body. Compatible with OpenClaw's frontmatter format. -pub(crate) fn parse_frontmatter( - content: &str, -) -> anyhow::Result<(HashMap, String)> { +/// Expects `---` delimiters. Content without frontmatter yields default +/// (empty) frontmatter and the full content as body. +pub(crate) fn parse_skill_markdown(content: &str) -> anyhow::Result<(SkillFrontmatter, String)> { let trimmed = content.trim_start(); if !trimmed.starts_with("---") { - // No frontmatter, entire content is body - return Ok((HashMap::new(), content.to_string())); + return Ok((SkillFrontmatter::default(), content.to_string())); } // Find the closing --- @@ -366,47 +555,17 @@ pub(crate) fn parse_frontmatter( anyhow::bail!("unclosed frontmatter: missing closing ---"); }; - let frontmatter_str = &after_opening[..end_pos].trim(); + let frontmatter_str = after_opening[..end_pos].trim(); let body_start = 3 + end_pos + 4; // skip opening --- + content + \n--- let body = trimmed[body_start..].trim_start_matches('\n').to_string(); - // Parse the YAML frontmatter into simple key-value pairs. - // We support the subset that OpenClaw uses: simple scalars and inline JSON for metadata. - let mut map = HashMap::new(); - - // Use serde_yaml-compatible line-based parsing for the simple cases - // (name, description, homepage, user-invocable, etc.) - // The metadata field can be complex JSON but we don't need to parse it — we just - // need name and description for Spacebot's purposes. - for line in frontmatter_str.lines() { - let line = line.trim(); - if line.is_empty() || line.starts_with('#') { - continue; - } - - // Handle simple `key: value` pairs - if let Some((key, value)) = line.split_once(':') { - let key = key.trim().to_string(); - let value = value.trim(); - - // Skip complex multi-line values (metadata JSON blocks, etc.) - if value.is_empty() || value.starts_with('{') || value.starts_with('[') { - continue; - } - - // Strip surrounding quotes - let value = value - .trim_start_matches('"') - .trim_end_matches('"') - .trim_start_matches('\'') - .trim_end_matches('\'') - .to_string(); - - map.insert(key, value); - } - } + let frontmatter = if frontmatter_str.is_empty() { + SkillFrontmatter::default() + } else { + serde_yaml::from_str(frontmatter_str).context("failed to parse skill frontmatter")? + }; - Ok((map, body)) + Ok((frontmatter, body)) } #[cfg(test)] @@ -427,13 +586,12 @@ mod tests { Two free services, no API keys needed. "#}; - let (fm, body) = parse_frontmatter(content).unwrap(); - assert_eq!(fm.get("name").unwrap(), "weather"); + let (fm, body) = parse_skill_markdown(content).unwrap(); + assert_eq!(fm.name.as_deref(), Some("weather")); assert_eq!( - fm.get("description").unwrap(), - "Get current weather and forecasts (no API key required)." + fm.description.as_deref(), + Some("Get current weather and forecasts (no API key required).") ); - assert_eq!(fm.get("homepage").unwrap(), "https://wttr.in/:help"); assert!(body.starts_with("# Weather")); } @@ -449,22 +607,21 @@ mod tests { # GitHub Skill "#}; - let (fm, body) = parse_frontmatter(content).unwrap(); - assert_eq!(fm.get("name").unwrap(), "github"); + let (fm, body) = parse_skill_markdown(content).unwrap(); + assert_eq!(fm.name.as_deref(), Some("github")); assert_eq!( - fm.get("description").unwrap(), - "Interact with GitHub using the gh CLI." + fm.description.as_deref(), + Some("Interact with GitHub using the gh CLI.") ); - // metadata line is skipped (starts with {) - assert!(!fm.contains_key("metadata")); assert!(body.starts_with("# GitHub Skill")); } #[test] fn test_parse_frontmatter_no_frontmatter() { let content = "# Just a markdown file\n\nNo frontmatter here."; - let (fm, body) = parse_frontmatter(content).unwrap(); - assert!(fm.is_empty()); + let (fm, body) = parse_skill_markdown(content).unwrap(); + assert!(fm.name.is_none()); + assert!(fm.description.is_none()); assert_eq!(body, content); } @@ -479,10 +636,90 @@ mod tests { Body here. "#}; - let (fm, _body) = parse_frontmatter(content).unwrap(); + let (fm, _body) = parse_skill_markdown(content).unwrap(); + assert_eq!( + fm.description.as_deref(), + Some("A skill with 'quotes' inside") + ); + } + + #[test] + fn test_parse_frontmatter_lists_and_multiline() { + let content = indoc::indoc! {r#" + --- + name: deploy + description: >- + Deploy the app to production + with the standard checklist. + tags: + - ops + - release + related_skills: [rollback, incident-response] + platforms: + - linux + - darwin + --- + + Body. + "#}; + + let (fm, _body) = parse_skill_markdown(content).unwrap(); + assert_eq!( + fm.description.as_deref(), + Some("Deploy the app to production with the standard checklist.") + ); + assert_eq!( + fm.tags.as_deref(), + Some(&["ops".to_string(), "release".to_string()][..]) + ); + assert_eq!( + fm.related_skills.as_deref(), + Some(&["rollback".to_string(), "incident-response".to_string()][..]) + ); + assert_eq!( + fm.platforms.as_deref(), + Some(&[Platform::Linux, Platform::Macos][..]) + ); + } + + #[test] + fn test_parse_frontmatter_unknown_platform_tolerated() { + let content = "---\nname: mobile\nplatforms: [android]\n---\n\nBody."; + let (fm, _body) = parse_skill_markdown(content).unwrap(); + assert_eq!(fm.platforms.as_deref(), Some(&[Platform::Other][..])); + } + + #[test] + fn test_index_description_truncated() { + let long = "x".repeat(DESCRIPTION_BUDGET + 40); + let rendered = index_description(&long); + assert!(rendered.chars().count() <= DESCRIPTION_BUDGET); + assert!(rendered.ends_with("...")); + + let short = "fits fine"; + assert_eq!(index_description(short), short); + } + + #[test] + fn test_index_description_budget_is_chars_not_bytes() { + // 79 two-byte chars: 158 bytes, but within the 80-char budget. + let multibyte = "é".repeat(DESCRIPTION_BUDGET - 1); + assert_eq!(index_description(&multibyte), multibyte); + + let over = "é".repeat(DESCRIPTION_BUDGET + 10); + let rendered = index_description(&over); + assert_eq!(rendered.chars().count(), DESCRIPTION_BUDGET); + assert!(rendered.ends_with("...")); + } + + #[test] + fn test_platform_other_never_matches_host() { + assert!(!Platform::list_matches_host(&[Platform::Other])); + assert!(!Platform::list_matches_host(&[])); + // The full list contains the host on any supported platform. assert_eq!( - fm.get("description").unwrap(), - "A skill with 'quotes' inside" + Platform::list_matches_host(&[Platform::Linux, Platform::Macos, Platform::Windows]), + Platform::host() != Platform::Other ); } @@ -506,6 +743,9 @@ mod tests { content: "# Weather\n\nUse curl.".into(), source: SkillSource::Instance, source_repo: None, + tags: Vec::new(), + related_skills: Vec::new(), + linked_files: Vec::new(), }, ); @@ -529,6 +769,9 @@ mod tests { content: "# Weather\n\nUse curl.".into(), source: SkillSource::Instance, source_repo: None, + tags: Vec::new(), + related_skills: Vec::new(), + linked_files: Vec::new(), }, ); @@ -560,6 +803,9 @@ mod tests { content: format!("# {name}"), source, source_repo: None, + tags: Vec::new(), + related_skills: Vec::new(), + linked_files: Vec::new(), } } diff --git a/src/skills/builtin.rs b/src/skills/builtin.rs index 8c9a3494e..c60f16cec 100644 --- a/src/skills/builtin.rs +++ b/src/skills/builtin.rs @@ -6,7 +6,7 @@ use std::path::PathBuf; -use super::{Skill, SkillSource, parse_frontmatter}; +use super::{Skill, SkillSource, parse_skill_markdown}; /// Each entry is `(directory_name, raw SKILL.md content)`. const BUILTIN_SKILLS: &[(&str, &str)] = &[( @@ -19,7 +19,7 @@ pub fn load() -> Vec { BUILTIN_SKILLS .iter() .filter_map(|(dir_name, raw)| match parse_builtin(dir_name, raw) { - Ok(skill) => Some(skill), + Ok(skill) => skill, Err(error) => { tracing::warn!( skill = %dir_name, @@ -32,25 +32,31 @@ pub fn load() -> Vec { .collect() } -fn parse_builtin(dir_name: &str, raw: &str) -> anyhow::Result { - let (frontmatter, body) = parse_frontmatter(raw)?; +/// Returns `Ok(None)` when the skill declares `platforms` that don't include +/// the host platform, matching filesystem skill loading. +fn parse_builtin(dir_name: &str, raw: &str) -> anyhow::Result> { + let (frontmatter, body) = parse_skill_markdown(raw)?; - let name = frontmatter - .get("name") - .cloned() - .unwrap_or_else(|| dir_name.to_string()); + if let Some(platforms) = &frontmatter.platforms + && !super::Platform::list_matches_host(platforms) + { + return Ok(None); + } - let description = frontmatter.get("description").cloned().unwrap_or_default(); + let name = frontmatter.name.unwrap_or_else(|| dir_name.to_string()); - Ok(Skill { + Ok(Some(Skill { name, - description, + description: frontmatter.description.unwrap_or_default(), file_path: PathBuf::from(format!("builtin://{dir_name}/SKILL.md")), base_dir: PathBuf::from(format!("builtin://{dir_name}")), content: body, source: SkillSource::Builtin, source_repo: None, - }) + tags: frontmatter.tags.unwrap_or_default(), + related_skills: frontmatter.related_skills.unwrap_or_default(), + linked_files: Vec::new(), + })) } #[cfg(test)] @@ -77,7 +83,7 @@ mod tests { #[test] fn parse_builtin_skill() { let raw = "---\nname: test-skill\ndescription: A test skill.\n---\n\n# Test\n\nBody here."; - let skill = parse_builtin("test-skill", raw).unwrap(); + let skill = parse_builtin("test-skill", raw).unwrap().unwrap(); assert_eq!(skill.name, "test-skill"); assert_eq!(skill.description, "A test skill."); assert_eq!(skill.source, SkillSource::Builtin); @@ -88,7 +94,15 @@ mod tests { #[test] fn parse_builtin_falls_back_to_dir_name() { let raw = "---\ndescription: No name field.\n---\n\nBody."; - let skill = parse_builtin("fallback-name", raw).unwrap(); + let skill = parse_builtin("fallback-name", raw).unwrap().unwrap(); assert_eq!(skill.name, "fallback-name"); } + + #[test] + fn parse_builtin_respects_platform_gate() { + // "android" deserializes to Platform::Other, which never matches any host. + let raw = "---\nname: mobile-only\ndescription: Gated.\nplatforms: [android]\n---\n\nBody."; + let skill = parse_builtin("mobile-only", raw).unwrap(); + assert!(skill.is_none()); + } } diff --git a/src/skills/installer.rs b/src/skills/installer.rs index a2365caf6..07755cff4 100644 --- a/src/skills/installer.rs +++ b/src/skills/installer.rs @@ -428,15 +428,12 @@ mod tests { #[test] fn test_inject_source_repo_roundtrip_with_parse() { - use crate::skills::parse_frontmatter; + use crate::skills::parse_skill_markdown; let content = "---\nname: weather\ndescription: Get weather\n---\n\n# Weather\n"; let patched = inject_source_repo(content, "anthropics/skills"); - let (fm, body) = parse_frontmatter(&patched).unwrap(); - assert_eq!( - fm.get("source_repo").unwrap(), - &"anthropics/skills".to_string() - ); - assert_eq!(fm.get("name").unwrap(), &"weather".to_string()); + let (fm, body) = parse_skill_markdown(&patched).unwrap(); + assert_eq!(fm.source_repo.as_deref(), Some("anthropics/skills")); + assert_eq!(fm.name.as_deref(), Some("weather")); assert!(body.contains("# Weather")); } } diff --git a/src/skills/usage.rs b/src/skills/usage.rs new file mode 100644 index 000000000..c41a141be --- /dev/null +++ b/src/skills/usage.rs @@ -0,0 +1,264 @@ +//! Per-skill provenance and usage tracking, backed by the agent's SQLite. +//! +//! Every skill gets a row in `skill_usage` recording who created it, how +//! often it's read and patched, and its lifecycle state. Curation only ever +//! operates on skills with `created_by = 'agent'` — user-authored and +//! registry-installed skills are outside curator jurisdiction unless the +//! user explicitly adopts them. + +use sqlx::{Row as _, SqlitePool}; + +/// Who initiated a skill write. Set by the process constructing the tool +/// server, never supplied by the model. Autonomous writers (`Agent`) get a +/// narrower blast radius: they can't touch installed, pinned, or +/// instance-level skills, and their deletes archive instead of remove. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum WriteOrigin { + /// A present human: channel conversations, API, CLI. + User, + /// An autonomous process: reflection branches, cortex curation. + Agent, +} + +/// A row from the `skill_usage` table. +#[derive(Debug, Clone)] +pub struct SkillUsageRecord { + pub skill_name: String, + pub created_by: String, + pub origin_conversation_id: Option, + pub state: String, + pub pinned: bool, + pub read_count: i64, + pub patch_count: i64, + pub last_read_at: Option, + pub last_patched_at: Option, + pub created_at: String, + pub archived_at: Option, +} + +/// Store for skill provenance and usage counters. +#[derive(Clone)] +pub struct SkillUsageStore { + pool: SqlitePool, +} + +impl std::fmt::Debug for SkillUsageStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SkillUsageStore").finish_non_exhaustive() + } +} + +impl SkillUsageStore { + pub fn new(pool: SqlitePool) -> Self { + Self { pool } + } + + /// Seed rows for skills that don't have one yet. + /// + /// Seeded skills get `created_by = 'user'` and `created_at = now` — a + /// newly noticed skill's staleness clock starts at discovery, not at + /// epoch, and unattributed skills default to the origin that protects + /// them from auto-curation. + pub async fn seed(&self, names: &[String]) -> anyhow::Result<()> { + let now = chrono::Utc::now().to_rfc3339(); + for name in names { + sqlx::query( + "INSERT OR IGNORE INTO skill_usage (skill_name, created_by, created_at) \ + VALUES (?, 'user', ?)", + ) + .bind(name.to_lowercase()) + .bind(&now) + .execute(&self.pool) + .await?; + } + Ok(()) + } + + /// Record a read: bump the counter, stamp the time, and reactivate a + /// stale skill. + pub async fn record_read(&self, name: &str) -> anyhow::Result<()> { + let key = name.to_lowercase(); + let now = chrono::Utc::now().to_rfc3339(); + + sqlx::query( + "INSERT OR IGNORE INTO skill_usage (skill_name, created_by, created_at) \ + VALUES (?, 'user', ?)", + ) + .bind(&key) + .bind(&now) + .execute(&self.pool) + .await?; + + sqlx::query( + "UPDATE skill_usage SET \ + read_count = read_count + 1, \ + last_read_at = ?, \ + state = CASE WHEN state = 'stale' THEN 'active' ELSE state END \ + WHERE skill_name = ?", + ) + .bind(&now) + .bind(&key) + .execute(&self.pool) + .await?; + + Ok(()) + } + + /// Mark skills as registry-installed, creating rows as needed. + /// + /// An install that replaces an existing row also clears any agent + /// conversation origin — the content on disk no longer comes from that + /// conversation. + pub async fn record_installed(&self, names: &[String]) -> anyhow::Result<()> { + let now = chrono::Utc::now().to_rfc3339(); + for name in names { + sqlx::query( + "INSERT INTO skill_usage (skill_name, created_by, created_at) \ + VALUES (?, 'installed', ?) \ + ON CONFLICT(skill_name) DO UPDATE SET \ + created_by = 'installed', \ + origin_conversation_id = NULL", + ) + .bind(name.to_lowercase()) + .bind(&now) + .execute(&self.pool) + .await?; + } + Ok(()) + } + + /// Drop the row for a removed skill. + pub async fn remove(&self, name: &str) -> anyhow::Result<()> { + sqlx::query("DELETE FROM skill_usage WHERE skill_name = ?") + .bind(name.to_lowercase()) + .execute(&self.pool) + .await?; + Ok(()) + } + + /// Fetch a single skill's usage record. + pub async fn get(&self, name: &str) -> anyhow::Result> { + let row = sqlx::query("SELECT * FROM skill_usage WHERE skill_name = ?") + .bind(name.to_lowercase()) + .fetch_optional(&self.pool) + .await?; + + Ok(row.map(record_from_row)) + } + + /// List all usage records, ordered by skill name. + pub async fn list(&self) -> anyhow::Result> { + let rows = sqlx::query("SELECT * FROM skill_usage ORDER BY skill_name") + .fetch_all(&self.pool) + .await?; + + Ok(rows.into_iter().map(record_from_row).collect()) + } +} + +fn record_from_row(row: sqlx::sqlite::SqliteRow) -> SkillUsageRecord { + SkillUsageRecord { + skill_name: row.get("skill_name"), + created_by: row.get("created_by"), + origin_conversation_id: row.get("origin_conversation_id"), + state: row.get("state"), + pinned: row.get::("pinned") != 0, + read_count: row.get("read_count"), + patch_count: row.get("patch_count"), + last_read_at: row.get("last_read_at"), + last_patched_at: row.get("last_patched_at"), + created_at: row.get("created_at"), + archived_at: row.get("archived_at"), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + async fn test_store() -> SkillUsageStore { + let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); + sqlx::migrate!("./migrations").run(&pool).await.unwrap(); + SkillUsageStore::new(pool) + } + + #[tokio::test] + async fn seed_is_idempotent_and_preserves_existing_rows() { + let store = test_store().await; + + store.seed(&["Weather".to_string()]).await.unwrap(); + store.record_read("weather").await.unwrap(); + store.seed(&["weather".to_string()]).await.unwrap(); + + let record = store.get("WEATHER").await.unwrap().unwrap(); + assert_eq!(record.skill_name, "weather"); + assert_eq!(record.created_by, "user"); + assert_eq!(record.read_count, 1); + } + + #[tokio::test] + async fn record_read_seeds_missing_row_and_reactivates_stale() { + let store = test_store().await; + + store.record_read("deploy").await.unwrap(); + let record = store.get("deploy").await.unwrap().unwrap(); + assert_eq!(record.read_count, 1); + assert!(record.last_read_at.is_some()); + + sqlx::query("UPDATE skill_usage SET state = 'stale' WHERE skill_name = 'deploy'") + .execute(&store.pool) + .await + .unwrap(); + + store.record_read("deploy").await.unwrap(); + let record = store.get("deploy").await.unwrap().unwrap(); + assert_eq!(record.state, "active"); + assert_eq!(record.read_count, 2); + } + + #[tokio::test] + async fn record_installed_overrides_seeded_provenance() { + let store = test_store().await; + + store.seed(&["github".to_string()]).await.unwrap(); + store + .record_installed(&["github".to_string()]) + .await + .unwrap(); + + let record = store.get("github").await.unwrap().unwrap(); + assert_eq!(record.created_by, "installed"); + } + + #[tokio::test] + async fn record_installed_clears_agent_conversation_origin() { + let store = test_store().await; + + sqlx::query( + "INSERT INTO skill_usage (skill_name, created_by, origin_conversation_id, created_at) \ + VALUES ('deploy', 'agent', 'conv-123', '2026-08-08T00:00:00Z')", + ) + .execute(&store.pool) + .await + .unwrap(); + + store + .record_installed(&["deploy".to_string()]) + .await + .unwrap(); + + let record = store.get("deploy").await.unwrap().unwrap(); + assert_eq!(record.created_by, "installed"); + assert!(record.origin_conversation_id.is_none()); + } + + #[tokio::test] + async fn remove_drops_the_row() { + let store = test_store().await; + + store.seed(&["temp".to_string()]).await.unwrap(); + store.remove("temp").await.unwrap(); + assert!(store.get("temp").await.unwrap().is_none()); + assert!(store.list().await.unwrap().is_empty()); + } +} diff --git a/src/tools.rs b/src/tools.rs index 620df58e8..c72ca64d0 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -516,6 +516,17 @@ pub async fn add_channel_tools( )) .await?; } + handle + .add_tool(SkillsSearchTool::new(state.deps.runtime_config.clone())) + .await?; + if let Some(api_state) = &state.deps.api_state { + handle + .add_tool(InstallSkillTool::new( + state.deps.runtime_config.clone(), + api_state.clone(), + )) + .await?; + } handle.add_tool(CancelTool::new(state)).await?; handle .add_tool(SkipTool::new(skip_flag.clone(), response_tx.clone())) @@ -768,6 +779,8 @@ pub async fn remove_channel_tools( let _ = handle.remove_tool(SendAgentMessageTool::NAME).await; let _ = handle.remove_tool(AttachmentRecallTool::NAME).await; let _ = handle.remove_tool(SetOutcomeTool::NAME).await; + let _ = handle.remove_tool(SkillsSearchTool::NAME).await; + let _ = handle.remove_tool(InstallSkillTool::NAME).await; Ok(()) } @@ -881,6 +894,7 @@ pub fn create_branch_tool_server( .tool(task_create) .tool(TaskListTool::new(task_store.clone(), agent_id.to_string())) .tool(TaskUpdateTool::for_branch(task_store, agent_id.clone())) + .tool(ReadSkillTool::new(runtime_config.clone())) .tool(FileReadTool::new( runtime_config.workspace_dir.clone(), sandbox, @@ -1067,8 +1081,8 @@ pub fn create_cortex_tool_server( /// `spacebot_docs` for embedded docs/changelog retrieval. /// /// **DEPRECATED:** Cortex chat is being replaced by Channel Settings. -/// Remaining unique tools here (skills_search, install_skill, config_inspect) -/// need to be ported to channel/worker toolsets before removal. +/// skills_search and install_skill are now also on the channel toolset; +/// config_inspect is the last unique tool to port before removal. #[allow(clippy::too_many_arguments)] #[deprecated( note = "Cortex chat is being replaced by Channel Settings. Port remaining tools before removing." diff --git a/src/tools/install_skill.rs b/src/tools/install_skill.rs index 44eba847a..9716e0674 100644 --- a/src/tools/install_skill.rs +++ b/src/tools/install_skill.rs @@ -1,6 +1,6 @@ -//! Install skill tool — lets cortex install skills from skills.sh into the agent workspace. +//! Install skill tool — install skills from skills.sh into the agent workspace. //! -//! After finding a skill via `skills_search`, cortex can install it directly +//! After finding a skill via `skills_search`, the agent can install it directly //! using this tool. Skills are installed to the agent's workspace skills directory //! and become immediately available to workers. @@ -132,6 +132,12 @@ impl Tool for InstallSkillTool { let skills = SkillSet::load(&instance_skills_dir, &target_dir).await; target_config.reload_skills(skills); + if let Some(store) = target_config.skill_usage.load().as_ref() + && let Err(error) = store.record_installed(&installed).await + { + tracing::warn!(%error, "failed to record installed skills"); + } + let agent_label = args.agent_id.as_deref().unwrap_or("current agent"); let names = installed.join(", "); Ok(InstallSkillOutput { diff --git a/src/tools/read_skill.rs b/src/tools/read_skill.rs index 2c2129e7b..2960b5bb6 100644 --- a/src/tools/read_skill.rs +++ b/src/tools/read_skill.rs @@ -41,6 +41,13 @@ pub struct ReadSkillArgs { pub struct ReadSkillOutput { /// The full skill instructions. pub content: String, + /// Files under the skill's support subdirectories (references/, templates/, + /// scripts/, assets/), relative to the skill directory. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub linked_files: Vec, + /// Names of related skills worth reading for this task class. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub related_skills: Vec, } impl Tool for ReadSkillTool { @@ -72,14 +79,26 @@ impl Tool for ReadSkillTool { async fn call(&self, args: Self::Args) -> Result { let skills = self.runtime_config.skills.load(); - match skills.get(&args.name) { - Some(skill) => Ok(ReadSkillOutput { - content: skill.content.clone(), - }), - None => Err(ReadSkillError(format!( + let Some(skill) = skills.get(&args.name) else { + return Err(ReadSkillError(format!( "skill '{}' not found. Available skills are listed in in your system prompt.", args.name - ))), + ))); + }; + + if let Some(store) = self.runtime_config.skill_usage.load().as_ref() + && let Err(error) = store.record_read(&skill.name).await + { + tracing::warn!(%error, skill = %skill.name, "failed to record skill read"); } + + Ok(ReadSkillOutput { + content: crate::tools::truncate_output( + &skill.content, + crate::tools::MAX_TOOL_OUTPUT_BYTES, + ), + linked_files: skill.linked_files.clone(), + related_skills: skill.related_skills.clone(), + }) } } diff --git a/src/tools/skills_search.rs b/src/tools/skills_search.rs index 7d21b3be4..813672dfa 100644 --- a/src/tools/skills_search.rs +++ b/src/tools/skills_search.rs @@ -1,12 +1,12 @@ -//! Skills search tool — lets cortex search the skills.sh registry and list installed skills. +//! Skills search tool — search the skills.sh registry and list installed skills. //! -//! Cortex chat needs to guide users through setting up integrations. This tool +//! Channels use this to guide users through setting up integrations. This tool //! provides two capabilities: //! //! 1. **Search the skills.sh registry** for skills matching a query (e.g. "github", -//! "aws", "docker"). This lets cortex recommend skills for users to install. +//! "aws", "docker"), to recommend skills for users to install. //! -//! 2. **List installed skills** on the current agent so cortex can see what's +//! 2. **List installed skills** on the current agent to see what's //! already available. use crate::config::RuntimeConfig; @@ -207,15 +207,14 @@ impl Tool for SkillsSearchTool { .await .map_err(|error| SkillsSearchError::RequestFailed(error.to_string()))?; - if !response.status().is_success() { + let status = response.status(); + if !status.is_success() { let body = response .text() .await .unwrap_or_else(|_| "failed to read response body".into()); return Err(SkillsSearchError::RequestFailed(format!( - "HTTP {}: {}", - body.len(), - body + "HTTP {status}: {body}" ))); }