feat: agent skillsの互換実装
This commit is contained in:
parent
5fe9a5805e
commit
760b304969
|
|
@ -15,8 +15,8 @@ use serde::{Deserialize, Serialize};
|
|||
use crate::defaults;
|
||||
use crate::model::{AuthRef, ModelManifest, ReasoningControl};
|
||||
use crate::{
|
||||
CompactionConfig, MemoryConfig, PodManifest, PodMeta, ScopeConfig, ToolOutputLimits,
|
||||
WorkerManifest,
|
||||
CompactionConfig, MemoryConfig, PodManifest, PodMeta, ScopeConfig, SkillsConfig,
|
||||
ToolOutputLimits, WorkerManifest,
|
||||
};
|
||||
|
||||
/// Partial-form Pod manifest. Every field is optional; one or more
|
||||
|
|
@ -41,6 +41,9 @@ pub struct PodManifestConfig {
|
|||
/// Memory subsystem opt-in. See [`MemoryConfig`].
|
||||
#[serde(default)]
|
||||
pub memory: Option<MemoryConfig>,
|
||||
/// External Agent Skills directories. See [`crate::SkillsConfig`].
|
||||
#[serde(default)]
|
||||
pub skills: Option<SkillsConfig>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
|
||||
|
|
@ -183,6 +186,11 @@ impl PodManifestConfig {
|
|||
{
|
||||
resolve_auth_file(&mut cp.auth, base);
|
||||
}
|
||||
if let Some(ref mut skills) = self.skills {
|
||||
for dir in &mut skills.directories {
|
||||
*dir = join_if_relative(base, dir);
|
||||
}
|
||||
}
|
||||
self
|
||||
}
|
||||
|
||||
|
|
@ -202,10 +210,18 @@ impl PodManifestConfig {
|
|||
CompactionConfigPartial::merge,
|
||||
),
|
||||
memory: merge_option(self.memory, upper.memory, MemoryConfig::merge),
|
||||
skills: merge_option(self.skills, upper.skills, SkillsConfig::merge),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl SkillsConfig {
|
||||
fn merge(mut self, upper: Self) -> Self {
|
||||
self.directories.extend(upper.directories);
|
||||
self
|
||||
}
|
||||
}
|
||||
|
||||
impl MemoryConfig {
|
||||
fn merge(self, upper: Self) -> Self {
|
||||
Self {
|
||||
|
|
@ -411,6 +427,12 @@ impl TryFrom<PodManifestConfig> for PodManifest {
|
|||
})
|
||||
.transpose()?;
|
||||
|
||||
if let Some(ref skills) = cfg.skills {
|
||||
for dir in &skills.directories {
|
||||
ensure_absolute("skills.directories", dir)?;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(PodManifest {
|
||||
pod: PodMeta { name, prompt_pack },
|
||||
model: cfg.model,
|
||||
|
|
@ -418,6 +440,7 @@ impl TryFrom<PodManifestConfig> for PodManifest {
|
|||
scope: cfg.scope,
|
||||
compaction,
|
||||
memory: cfg.memory,
|
||||
skills: cfg.skills,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -461,6 +484,7 @@ mod tests {
|
|||
},
|
||||
compaction: None,
|
||||
memory: None,
|
||||
skills: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -892,6 +916,73 @@ name = "dbg"
|
|||
assert_eq!(manifest.scope.allow.len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skills_directories_resolved_against_base() {
|
||||
let mut cfg = minimal_valid();
|
||||
cfg.skills = Some(SkillsConfig {
|
||||
directories: vec![PathBuf::from(".claude/skills"), PathBuf::from("/abs/elsewhere")],
|
||||
});
|
||||
let resolved = cfg.resolve_paths(Path::new("/workspace/proj"));
|
||||
let dirs = resolved.skills.as_ref().unwrap().directories.clone();
|
||||
assert_eq!(dirs[0], PathBuf::from("/workspace/proj/.claude/skills"));
|
||||
assert_eq!(dirs[1], PathBuf::from("/abs/elsewhere"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skills_relative_path_rejected_post_resolve() {
|
||||
let mut cfg = minimal_valid();
|
||||
cfg.skills = Some(SkillsConfig {
|
||||
directories: vec![PathBuf::from("relative/skills")],
|
||||
});
|
||||
let err = PodManifest::try_from(cfg).unwrap_err();
|
||||
assert!(matches!(
|
||||
err,
|
||||
ResolveError::RelativePath {
|
||||
field: "skills.directories",
|
||||
..
|
||||
}
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skills_merge_extends_directories() {
|
||||
let lower = PodManifestConfig {
|
||||
skills: Some(SkillsConfig {
|
||||
directories: vec![PathBuf::from("/a")],
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
let upper = PodManifestConfig {
|
||||
skills: Some(SkillsConfig {
|
||||
directories: vec![PathBuf::from("/b")],
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
let merged = lower.merge(upper);
|
||||
let dirs = merged.skills.unwrap().directories;
|
||||
assert_eq!(dirs, vec![PathBuf::from("/a"), PathBuf::from("/b")]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn from_toml_parses_skills_section() {
|
||||
let toml = r#"
|
||||
[pod]
|
||||
name = "x"
|
||||
|
||||
[skills]
|
||||
directories = [".claude/skills", ".cursor/skills"]
|
||||
"#;
|
||||
let cfg = PodManifestConfig::from_toml(toml).unwrap();
|
||||
let dirs = cfg.skills.unwrap().directories;
|
||||
assert_eq!(
|
||||
dirs,
|
||||
vec![
|
||||
PathBuf::from(".claude/skills"),
|
||||
PathBuf::from(".cursor/skills"),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_preserves_ref() {
|
||||
let lower = PodManifestConfig {
|
||||
|
|
|
|||
|
|
@ -44,6 +44,30 @@ pub struct PodManifest {
|
|||
/// memory tools registered.
|
||||
#[serde(default)]
|
||||
pub memory: Option<MemoryConfig>,
|
||||
/// External Agent Skills (`SKILL.md`) directories to ingest as
|
||||
/// Workflows in addition to the user-level `$config_dir/skills/`.
|
||||
/// Each entry is a path to a skills *root* (i.e. a directory whose
|
||||
/// children are individual `<name>/SKILL.md` skill bundles). Paths
|
||||
/// are resolved against the manifest's base directory like other
|
||||
/// path fields.
|
||||
#[serde(default)]
|
||||
pub skills: Option<SkillsConfig>,
|
||||
}
|
||||
|
||||
/// External Agent Skills (`SKILL.md`) ingest configuration. Off by
|
||||
/// default at the workspace level; user-level `$config_dir/skills/` is
|
||||
/// always probed regardless of this field. The intent of `directories`
|
||||
/// is to surface skills that already live in `.claude/skills/`,
|
||||
/// `.cursor/skills/`, etc. without duplicating them under the insomnia
|
||||
/// memory tree.
|
||||
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
|
||||
pub struct SkillsConfig {
|
||||
/// Skills *roots*. Children of each root must be individual
|
||||
/// `<name>/SKILL.md` bundles; the directory itself is not a skill.
|
||||
/// Resolved against the manifest base directory before
|
||||
/// [`PodManifest`] is materialised.
|
||||
#[serde(default)]
|
||||
pub directories: Vec<PathBuf>,
|
||||
}
|
||||
|
||||
/// Memory subsystem configuration. Presence in the manifest enables
|
||||
|
|
|
|||
|
|
@ -79,6 +79,12 @@ pub fn user_prompts_dir() -> Option<PathBuf> {
|
|||
Some(config_dir()?.join("prompts"))
|
||||
}
|
||||
|
||||
/// `<config_dir>/skills/` — user-level Agent Skills ライブラリ。
|
||||
/// 配下は `<name>/SKILL.md` の集合として読まれる。
|
||||
pub fn user_skills_dir() -> Option<PathBuf> {
|
||||
Some(config_dir()?.join("skills"))
|
||||
}
|
||||
|
||||
/// `<config_dir>/prompts.toml` — user prompt pack。
|
||||
pub fn user_pack_file() -> Option<PathBuf> {
|
||||
Some(config_dir()?.join("prompts.toml"))
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ pub mod linter;
|
|||
pub mod resident;
|
||||
pub mod schema;
|
||||
pub mod scope;
|
||||
pub mod skill;
|
||||
pub mod slug;
|
||||
pub mod tool;
|
||||
pub mod workflow;
|
||||
|
|
@ -23,9 +24,10 @@ pub use extract::ExtractPointerPayload;
|
|||
pub use linter::{LintReport, Linter};
|
||||
pub use resident::{ResidentKnowledgeEntry, collect_resident_knowledge};
|
||||
pub use scope::deny_write_rules;
|
||||
pub use skill::{SKILL_FILENAME, SkillParseError, SkillRecord, load_skills_from_dir, parse_skill_md};
|
||||
pub use slug::Slug;
|
||||
pub use workflow::{
|
||||
ResidentWorkflowEntry, WORKFLOW_DESCRIPTION_HARD_CAP, WorkflowLoadError, WorkflowRecord,
|
||||
WorkflowRegistry, load_workflows,
|
||||
ResidentWorkflowEntry, ShadowedSkill, WORKFLOW_DESCRIPTION_HARD_CAP, WorkflowLoadError,
|
||||
WorkflowRecord, WorkflowRegistry, WorkflowSource, load_workflows,
|
||||
};
|
||||
pub use workspace::WorkspaceLayout;
|
||||
|
|
|
|||
447
crates/memory/src/skill.rs
Normal file
447
crates/memory/src/skill.rs
Normal file
|
|
@ -0,0 +1,447 @@
|
|||
//! Agent Skills (`SKILL.md`) parser.
|
||||
//!
|
||||
//! Skills follow the [agentskills.io](https://agentskills.io/specification)
|
||||
//! spec: a directory `<root>/<name>/` containing `SKILL.md` (YAML frontmatter
|
||||
//! + Markdown body) and optional `scripts/` / `references/` / `assets/`
|
||||
//! subdirectories. The body is procedural agent guidance; insomnia ingests
|
||||
//! it as a Workflow so `/<name>` resolves to it just like an internal
|
||||
//! Workflow.
|
||||
//!
|
||||
//! Parsing is intentionally lenient at the directory-scan level — one
|
||||
//! malformed SKILL.md emits `tracing::warn!` and is skipped, leaving sibling
|
||||
//! skills loadable. Internal Workflows (`memory/workflow/<slug>.md`) keep
|
||||
//! their hard-error semantics.
|
||||
|
||||
use std::io;
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use serde::Deserialize;
|
||||
use thiserror::Error;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::error::LintError;
|
||||
use crate::schema::split_frontmatter;
|
||||
use crate::slug::Slug;
|
||||
use crate::workflow::{WORKFLOW_DESCRIPTION_HARD_CAP, WorkflowRecord, WorkflowSource};
|
||||
|
||||
/// Filename within a skill directory carrying the frontmatter + body.
|
||||
pub const SKILL_FILENAME: &str = "SKILL.md";
|
||||
|
||||
/// SKILL.md frontmatter as defined by the agent-skills spec.
|
||||
///
|
||||
/// Fields beyond `name` / `description` are accepted to be spec-compatible
|
||||
/// but not used by insomnia today: `license`, `compatibility`, and
|
||||
/// `metadata` are documentary, while `allowed-tools` is recognised and
|
||||
/// emits a warning until [`permission-extension-point.md`] lands.
|
||||
#[derive(Debug, Clone, Deserialize)]
|
||||
pub struct SkillFrontmatter {
|
||||
pub name: String,
|
||||
pub description: String,
|
||||
#[serde(default)]
|
||||
pub license: Option<String>,
|
||||
#[serde(default)]
|
||||
pub compatibility: Option<String>,
|
||||
#[serde(default)]
|
||||
pub metadata: Option<serde_yaml::Value>,
|
||||
#[serde(default, rename = "allowed-tools")]
|
||||
pub allowed_tools: Option<serde_yaml::Value>,
|
||||
}
|
||||
|
||||
/// Validated skill record. Constructed by [`parse_skill_md`] and converted
|
||||
/// to a `WorkflowRecord` by the caller via the `Skill → Workflow`
|
||||
/// projection in [`crate::workflow`].
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct SkillRecord {
|
||||
pub slug: Slug,
|
||||
pub description: String,
|
||||
pub body: String,
|
||||
/// The skill directory (parent of `SKILL.md`). Carried so callers can
|
||||
/// register `scripts/` / `references/` / `assets/` against the Pod's
|
||||
/// scope.
|
||||
pub dir: PathBuf,
|
||||
/// Path to the `SKILL.md` file itself. Used as the resolved path on
|
||||
/// the resulting `WorkflowRecord`.
|
||||
pub skill_md_path: PathBuf,
|
||||
}
|
||||
|
||||
impl SkillRecord {
|
||||
/// Project this skill into a [`WorkflowRecord`]. Skill-sourced
|
||||
/// Workflows are advertised resident (`model_invokation: true`,
|
||||
/// matching the agentskills progressive-disclosure model), are
|
||||
/// invocable as `/<slug>`, and carry no `requires` since the SKILL
|
||||
/// spec has no Knowledge-dependency concept.
|
||||
pub fn into_workflow_record(self, source: WorkflowSource) -> WorkflowRecord {
|
||||
WorkflowRecord {
|
||||
slug: self.slug,
|
||||
description: self.description,
|
||||
model_invokation: true,
|
||||
user_invocable: true,
|
||||
requires: Vec::new(),
|
||||
body: self.body,
|
||||
path: self.skill_md_path,
|
||||
source,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
pub enum SkillParseError {
|
||||
#[error("skill path has no parent directory: {}", .0.display())]
|
||||
NoParentDir(PathBuf),
|
||||
#[error("failed to read SKILL.md at {}: {source}", .path.display())]
|
||||
ReadFile { path: PathBuf, source: io::Error },
|
||||
#[error("invalid frontmatter in {}: {source}", .path.display())]
|
||||
Frontmatter {
|
||||
path: PathBuf,
|
||||
#[source]
|
||||
source: LintError,
|
||||
},
|
||||
#[error(
|
||||
"SKILL.md `name` `{name}` does not match its directory name `{dir_name}` (at {})",
|
||||
.skill_md_path.display()
|
||||
)]
|
||||
NameDirMismatch {
|
||||
name: String,
|
||||
dir_name: String,
|
||||
skill_md_path: PathBuf,
|
||||
},
|
||||
#[error("SKILL.md `name` is not a valid slug at {}: {source}", .skill_md_path.display())]
|
||||
InvalidName {
|
||||
skill_md_path: PathBuf,
|
||||
#[source]
|
||||
source: LintError,
|
||||
},
|
||||
#[error("SKILL.md `description` must be non-empty (at {})", .skill_md_path.display())]
|
||||
DescriptionEmpty { skill_md_path: PathBuf },
|
||||
#[error(
|
||||
"SKILL.md `description` length {actual} exceeds limit {limit} (at {})",
|
||||
.skill_md_path.display()
|
||||
)]
|
||||
DescriptionTooLong {
|
||||
skill_md_path: PathBuf,
|
||||
actual: usize,
|
||||
limit: usize,
|
||||
},
|
||||
}
|
||||
|
||||
/// Parse a single `SKILL.md`. The directory name is taken from the parent
|
||||
/// of `skill_md_path` and validated against the frontmatter `name`.
|
||||
pub fn parse_skill_md(skill_md_path: &Path) -> Result<SkillRecord, SkillParseError> {
|
||||
let dir = skill_md_path
|
||||
.parent()
|
||||
.map(|p| p.to_path_buf())
|
||||
.ok_or_else(|| SkillParseError::NoParentDir(skill_md_path.to_path_buf()))?;
|
||||
let dir_name = dir
|
||||
.file_name()
|
||||
.and_then(|s| s.to_str())
|
||||
.map(|s| s.to_string())
|
||||
.ok_or_else(|| SkillParseError::NoParentDir(skill_md_path.to_path_buf()))?;
|
||||
|
||||
let raw =
|
||||
std::fs::read_to_string(skill_md_path).map_err(|source| SkillParseError::ReadFile {
|
||||
path: skill_md_path.to_path_buf(),
|
||||
source,
|
||||
})?;
|
||||
let (yaml, body) = split_frontmatter(&raw).map_err(|source| SkillParseError::Frontmatter {
|
||||
path: skill_md_path.to_path_buf(),
|
||||
source,
|
||||
})?;
|
||||
warn_unknown_skill_fields(skill_md_path, yaml);
|
||||
let frontmatter: SkillFrontmatter =
|
||||
serde_yaml::from_str(yaml).map_err(|err| SkillParseError::Frontmatter {
|
||||
path: skill_md_path.to_path_buf(),
|
||||
source: LintError::MalformedFrontmatter(err.to_string()),
|
||||
})?;
|
||||
|
||||
if frontmatter.allowed_tools.is_some() {
|
||||
warn!(
|
||||
path = %skill_md_path.display(),
|
||||
"SKILL.md `allowed-tools` is recognised but not yet enforced; ignoring"
|
||||
);
|
||||
}
|
||||
|
||||
let desc_chars = frontmatter.description.chars().count();
|
||||
if desc_chars == 0 {
|
||||
return Err(SkillParseError::DescriptionEmpty {
|
||||
skill_md_path: skill_md_path.to_path_buf(),
|
||||
});
|
||||
}
|
||||
if desc_chars > WORKFLOW_DESCRIPTION_HARD_CAP {
|
||||
return Err(SkillParseError::DescriptionTooLong {
|
||||
skill_md_path: skill_md_path.to_path_buf(),
|
||||
actual: desc_chars,
|
||||
limit: WORKFLOW_DESCRIPTION_HARD_CAP,
|
||||
});
|
||||
}
|
||||
|
||||
if frontmatter.name != dir_name {
|
||||
return Err(SkillParseError::NameDirMismatch {
|
||||
name: frontmatter.name,
|
||||
dir_name,
|
||||
skill_md_path: skill_md_path.to_path_buf(),
|
||||
});
|
||||
}
|
||||
let slug = Slug::parse(frontmatter.name).map_err(|source| SkillParseError::InvalidName {
|
||||
skill_md_path: skill_md_path.to_path_buf(),
|
||||
source,
|
||||
})?;
|
||||
|
||||
Ok(SkillRecord {
|
||||
slug,
|
||||
description: frontmatter.description,
|
||||
body: body.to_string(),
|
||||
dir,
|
||||
skill_md_path: skill_md_path.to_path_buf(),
|
||||
})
|
||||
}
|
||||
|
||||
/// Scan a skills root for `<root>/<name>/SKILL.md`. Returns successfully
|
||||
/// parsed skills; per-skill errors emit a `tracing::warn!` and are
|
||||
/// skipped. A missing root is treated as zero skills, not an error —
|
||||
/// callers can probe optional directories without pre-checking.
|
||||
pub fn load_skills_from_dir(root: &Path) -> Vec<SkillRecord> {
|
||||
let entries = match std::fs::read_dir(root) {
|
||||
Ok(it) => it,
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => return Vec::new(),
|
||||
Err(err) => {
|
||||
warn!(
|
||||
dir = %root.display(),
|
||||
error = %err,
|
||||
"failed to read skills directory; treating as empty"
|
||||
);
|
||||
return Vec::new();
|
||||
}
|
||||
};
|
||||
|
||||
let mut paths: Vec<PathBuf> = Vec::new();
|
||||
for entry in entries {
|
||||
let entry = match entry {
|
||||
Ok(e) => e,
|
||||
Err(err) => {
|
||||
warn!(
|
||||
dir = %root.display(),
|
||||
error = %err,
|
||||
"skill directory entry read error; skipping"
|
||||
);
|
||||
continue;
|
||||
}
|
||||
};
|
||||
let path = entry.path();
|
||||
if !path.is_dir() {
|
||||
continue;
|
||||
}
|
||||
let skill_md = path.join(SKILL_FILENAME);
|
||||
if skill_md.is_file() {
|
||||
paths.push(skill_md);
|
||||
}
|
||||
}
|
||||
paths.sort();
|
||||
|
||||
let mut out = Vec::new();
|
||||
for path in paths {
|
||||
match parse_skill_md(&path) {
|
||||
Ok(record) => out.push(record),
|
||||
Err(err) => warn!(path = %path.display(), error = %err, "SKILL.md skipped"),
|
||||
}
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
fn warn_unknown_skill_fields(path: &Path, yaml: &str) {
|
||||
let Ok(value) = serde_yaml::from_str::<serde_yaml::Value>(yaml) else {
|
||||
return;
|
||||
};
|
||||
let Some(map) = value.as_mapping() else {
|
||||
return;
|
||||
};
|
||||
for key in map.keys().filter_map(|k| k.as_str()) {
|
||||
if !matches!(
|
||||
key,
|
||||
"name" | "description" | "license" | "compatibility" | "metadata" | "allowed-tools"
|
||||
) {
|
||||
warn!(path = %path.display(), field = key, "unknown SKILL.md frontmatter field ignored");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use tempfile::TempDir;
|
||||
|
||||
fn write_skill(root: &Path, name: &str, frontmatter: &str, body: &str) -> PathBuf {
|
||||
let dir = root.join(name);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let path = dir.join(SKILL_FILENAME);
|
||||
std::fs::write(&path, format!("---\n{frontmatter}\n---\n{body}")).unwrap();
|
||||
path
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_minimal_skill() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"do-thing",
|
||||
"name: do-thing\ndescription: Do the thing",
|
||||
"Step 1\nStep 2\n",
|
||||
);
|
||||
let record = parse_skill_md(&path).unwrap();
|
||||
assert_eq!(record.slug.as_str(), "do-thing");
|
||||
assert_eq!(record.description, "Do the thing");
|
||||
assert_eq!(record.body, "Step 1\nStep 2\n");
|
||||
assert_eq!(record.dir, dir.path().join("do-thing"));
|
||||
assert_eq!(record.skill_md_path, path);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn name_dir_mismatch_is_error() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"actual-dir",
|
||||
"name: declared-name\ndescription: x",
|
||||
"body",
|
||||
);
|
||||
let err = parse_skill_md(&path).unwrap_err();
|
||||
assert!(matches!(err, SkillParseError::NameDirMismatch { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn invalid_slug_name_is_error() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"BAD-Caps",
|
||||
"name: BAD-Caps\ndescription: x",
|
||||
"body",
|
||||
);
|
||||
// Slug::parse rejects uppercase before the dir match check fires;
|
||||
// either way the parse is rejected.
|
||||
let err = parse_skill_md(&path).unwrap_err();
|
||||
assert!(matches!(
|
||||
err,
|
||||
SkillParseError::InvalidName { .. } | SkillParseError::NameDirMismatch { .. }
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_description_is_error() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = write_skill(dir.path(), "x", "name: x\ndescription: \"\"", "body");
|
||||
let err = parse_skill_md(&path).unwrap_err();
|
||||
assert!(matches!(err, SkillParseError::DescriptionEmpty { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn description_at_cap_is_accepted() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let desc = "x".repeat(WORKFLOW_DESCRIPTION_HARD_CAP);
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"x",
|
||||
&format!("name: x\ndescription: {desc}"),
|
||||
"body",
|
||||
);
|
||||
let record = parse_skill_md(&path).unwrap();
|
||||
assert_eq!(record.description.chars().count(), WORKFLOW_DESCRIPTION_HARD_CAP);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn description_over_cap_is_error() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let desc = "x".repeat(WORKFLOW_DESCRIPTION_HARD_CAP + 1);
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"x",
|
||||
&format!("name: x\ndescription: {desc}"),
|
||||
"body",
|
||||
);
|
||||
let err = parse_skill_md(&path).unwrap_err();
|
||||
assert!(matches!(err, SkillParseError::DescriptionTooLong { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn missing_frontmatter_is_error() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = dir.path().join("x").join(SKILL_FILENAME);
|
||||
std::fs::create_dir_all(path.parent().unwrap()).unwrap();
|
||||
std::fs::write(&path, "no frontmatter at all\n").unwrap();
|
||||
let err = parse_skill_md(&path).unwrap_err();
|
||||
assert!(matches!(err, SkillParseError::Frontmatter { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extra_frontmatter_fields_are_kept() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"x",
|
||||
"name: x\ndescription: ok\nlicense: MIT\ncompatibility: claude-4\n\
|
||||
metadata:\n team: foo\nallowed-tools:\n - Read",
|
||||
"body",
|
||||
);
|
||||
let record = parse_skill_md(&path).unwrap();
|
||||
assert_eq!(record.slug.as_str(), "x");
|
||||
// allowed-tools triggers a warn, but parse succeeds.
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_skills_from_dir_skips_broken_and_keeps_good() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
write_skill(dir.path(), "good", "name: good\ndescription: ok", "body");
|
||||
// Mismatch — should be skipped, not abort the scan.
|
||||
write_skill(
|
||||
dir.path(),
|
||||
"bad-dir",
|
||||
"name: declared-different\ndescription: ok",
|
||||
"body",
|
||||
);
|
||||
// A bare file at the root (not a directory) is ignored.
|
||||
std::fs::write(dir.path().join("stray.md"), "not a skill").unwrap();
|
||||
|
||||
let records = load_skills_from_dir(dir.path());
|
||||
let slugs: Vec<&str> = records.iter().map(|r| r.slug.as_str()).collect();
|
||||
assert_eq!(slugs, vec!["good"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_skills_from_dir_missing_root_is_empty() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let records = load_skills_from_dir(&dir.path().join("does-not-exist"));
|
||||
assert!(records.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn into_workflow_record_uses_skill_defaults() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = write_skill(
|
||||
dir.path(),
|
||||
"x",
|
||||
"name: x\ndescription: Project X",
|
||||
"Steps\n",
|
||||
);
|
||||
let record = parse_skill_md(&path).unwrap();
|
||||
let wf = record.into_workflow_record(WorkflowSource::UserSkill {
|
||||
dir: dir.path().to_path_buf(),
|
||||
});
|
||||
assert_eq!(wf.slug.as_str(), "x");
|
||||
assert_eq!(wf.description, "Project X");
|
||||
assert!(wf.model_invokation);
|
||||
assert!(wf.user_invocable);
|
||||
assert!(wf.requires.is_empty());
|
||||
assert_eq!(wf.body, "Steps\n");
|
||||
assert!(matches!(wf.source, WorkflowSource::UserSkill { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_skills_from_dir_orders_deterministically() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
write_skill(dir.path(), "b", "name: b\ndescription: b", "");
|
||||
write_skill(dir.path(), "a", "name: a\ndescription: a", "");
|
||||
write_skill(dir.path(), "c", "name: c\ndescription: c", "");
|
||||
let records = load_skills_from_dir(dir.path());
|
||||
let slugs: Vec<&str> = records.iter().map(|r| r.slug.as_str()).collect();
|
||||
assert_eq!(slugs, vec!["a", "b", "c"]);
|
||||
}
|
||||
}
|
||||
|
|
@ -21,6 +21,34 @@ use crate::workspace::WorkspaceLayout;
|
|||
/// Mirrors agent-skills and resident Knowledge descriptions.
|
||||
pub const WORKFLOW_DESCRIPTION_HARD_CAP: usize = 1024;
|
||||
|
||||
/// Origin of a [`WorkflowRecord`]. Used to break ties when the same slug
|
||||
/// is provided by multiple sources: workspace-authored Workflows always
|
||||
/// win over external skills, and workspace skills win over user skills.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum WorkflowSource {
|
||||
/// `<workspace>/.insomnia/memory/workflow/<slug>.md`. Authored
|
||||
/// in-tree by the project.
|
||||
WorkspaceWorkflow,
|
||||
/// SKILL.md ingested from a `[skills] directories` entry in the
|
||||
/// project manifest. `dir` is the skills root that contained
|
||||
/// `<slug>/SKILL.md`.
|
||||
WorkspaceSkill { dir: PathBuf },
|
||||
/// SKILL.md ingested from `$user/skills/`. `dir` is the user-level
|
||||
/// skills root.
|
||||
UserSkill { dir: PathBuf },
|
||||
}
|
||||
|
||||
impl WorkflowSource {
|
||||
/// Human-readable label used in shadow-notification messages.
|
||||
pub fn label(&self) -> &'static str {
|
||||
match self {
|
||||
Self::WorkspaceWorkflow => "workspace workflow",
|
||||
Self::WorkspaceSkill { .. } => "workspace skill",
|
||||
Self::UserSkill { .. } => "user skill",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct WorkflowRecord {
|
||||
pub slug: Slug,
|
||||
|
|
@ -31,6 +59,37 @@ pub struct WorkflowRecord {
|
|||
/// Markdown body after the closing frontmatter delimiter.
|
||||
pub body: String,
|
||||
pub path: PathBuf,
|
||||
/// Where this record was loaded from. Determines shadowing priority
|
||||
/// when [`WorkflowRegistry::merge_skill`] encounters a slug
|
||||
/// collision.
|
||||
pub source: WorkflowSource,
|
||||
}
|
||||
|
||||
/// Returned by [`WorkflowRegistry::merge_skill`] when an incoming skill is
|
||||
/// shadowed by an existing record (either an internal Workflow or a
|
||||
/// higher-priority skill). Carries enough context for a `Notification` to
|
||||
/// explain which side won.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct ShadowedSkill {
|
||||
pub slug: Slug,
|
||||
pub kept_source: WorkflowSource,
|
||||
pub kept_path: PathBuf,
|
||||
pub shadowed_source: WorkflowSource,
|
||||
pub shadowed_path: PathBuf,
|
||||
}
|
||||
|
||||
impl ShadowedSkill {
|
||||
/// One-line message for `Notification` payloads.
|
||||
pub fn message(&self) -> String {
|
||||
format!(
|
||||
"skill /{slug} from {shadowed_label} ({shadowed_path}) was shadowed by existing {kept_label} ({kept_path})",
|
||||
slug = self.slug,
|
||||
shadowed_label = self.shadowed_source.label(),
|
||||
shadowed_path = self.shadowed_path.display(),
|
||||
kept_label = self.kept_source.label(),
|
||||
kept_path = self.kept_path.display(),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default)]
|
||||
|
|
@ -77,6 +136,26 @@ impl WorkflowRegistry {
|
|||
.map(|record| record.slug.to_string())
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Insert a skill-derived record. If an existing record (internal
|
||||
/// Workflow or higher-priority skill) already owns the slug, the
|
||||
/// incoming record is dropped and a [`ShadowedSkill`] describing the
|
||||
/// collision is returned. Callers must invoke this in
|
||||
/// **descending-priority order** (workspace skills before user
|
||||
/// skills); the registry does not re-rank afterwards.
|
||||
pub fn merge_skill(&mut self, record: WorkflowRecord) -> Option<ShadowedSkill> {
|
||||
if let Some(existing) = self.records.get(&record.slug) {
|
||||
return Some(ShadowedSkill {
|
||||
slug: record.slug.clone(),
|
||||
kept_source: existing.source.clone(),
|
||||
kept_path: existing.path.clone(),
|
||||
shadowed_source: record.source,
|
||||
shadowed_path: record.path,
|
||||
});
|
||||
}
|
||||
self.records.insert(record.slug.clone(), record);
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
|
|
@ -176,6 +255,7 @@ pub fn load_workflows(layout: &WorkspaceLayout) -> Result<WorkflowRegistry, Work
|
|||
requires: frontmatter.requires,
|
||||
body: body.to_string(),
|
||||
path: path.clone(),
|
||||
source: WorkflowSource::WorkspaceWorkflow,
|
||||
};
|
||||
records.insert(slug.clone(), record);
|
||||
}
|
||||
|
|
@ -297,6 +377,114 @@ mod tests {
|
|||
assert!(matches!(err, WorkflowLoadError::Frontmatter { .. }));
|
||||
}
|
||||
|
||||
fn skill_record(slug: &str, path: &Path) -> WorkflowRecord {
|
||||
WorkflowRecord {
|
||||
slug: Slug::parse(slug).unwrap(),
|
||||
description: format!("desc {slug}"),
|
||||
model_invokation: true,
|
||||
user_invocable: true,
|
||||
requires: Vec::new(),
|
||||
body: format!("body for {slug}"),
|
||||
path: path.to_path_buf(),
|
||||
source: WorkflowSource::WorkspaceSkill {
|
||||
dir: path.parent().unwrap().parent().unwrap().to_path_buf(),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_skill_inserts_when_no_collision() {
|
||||
let mut reg = WorkflowRegistry::empty();
|
||||
let path = std::path::PathBuf::from("/tmp/skills/x/SKILL.md");
|
||||
let shadow = reg.merge_skill(skill_record("x", &path));
|
||||
assert!(shadow.is_none());
|
||||
assert_eq!(reg.len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_skill_shadows_existing_workflow() {
|
||||
let (dir, layout) = setup();
|
||||
write_workflow(dir.path(), "shared", "description: Internal", "internal body");
|
||||
let mut reg = load_workflows(&layout).unwrap();
|
||||
let skill_path = dir.path().join("user-skills").join("shared").join("SKILL.md");
|
||||
std::fs::create_dir_all(skill_path.parent().unwrap()).unwrap();
|
||||
std::fs::write(&skill_path, "ignored").unwrap();
|
||||
let incoming = WorkflowRecord {
|
||||
slug: Slug::parse("shared").unwrap(),
|
||||
description: "From skill".into(),
|
||||
model_invokation: true,
|
||||
user_invocable: true,
|
||||
requires: Vec::new(),
|
||||
body: "skill body".into(),
|
||||
path: skill_path.clone(),
|
||||
source: WorkflowSource::UserSkill {
|
||||
dir: dir.path().join("user-skills"),
|
||||
},
|
||||
};
|
||||
let shadow = reg.merge_skill(incoming).expect("expected shadow");
|
||||
assert_eq!(shadow.slug.as_str(), "shared");
|
||||
assert!(matches!(shadow.kept_source, WorkflowSource::WorkspaceWorkflow));
|
||||
assert!(matches!(shadow.shadowed_source, WorkflowSource::UserSkill { .. }));
|
||||
// The kept record is still the workspace workflow.
|
||||
let kept = reg.get(&Slug::parse("shared").unwrap()).unwrap();
|
||||
assert!(matches!(kept.source, WorkflowSource::WorkspaceWorkflow));
|
||||
assert_eq!(kept.body, "internal body");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_skill_priority_workspace_over_user() {
|
||||
let mut reg = WorkflowRegistry::empty();
|
||||
let ws_path = std::path::PathBuf::from("/ws/skills/x/SKILL.md");
|
||||
let user_path = std::path::PathBuf::from("/user/skills/x/SKILL.md");
|
||||
let ws_record = WorkflowRecord {
|
||||
slug: Slug::parse("x").unwrap(),
|
||||
description: "ws".into(),
|
||||
model_invokation: true,
|
||||
user_invocable: true,
|
||||
requires: Vec::new(),
|
||||
body: "ws body".into(),
|
||||
path: ws_path.clone(),
|
||||
source: WorkflowSource::WorkspaceSkill {
|
||||
dir: std::path::PathBuf::from("/ws/skills"),
|
||||
},
|
||||
};
|
||||
let user_record = WorkflowRecord {
|
||||
slug: Slug::parse("x").unwrap(),
|
||||
description: "user".into(),
|
||||
model_invokation: true,
|
||||
user_invocable: true,
|
||||
requires: Vec::new(),
|
||||
body: "user body".into(),
|
||||
path: user_path.clone(),
|
||||
source: WorkflowSource::UserSkill {
|
||||
dir: std::path::PathBuf::from("/user/skills"),
|
||||
},
|
||||
};
|
||||
// Caller is required to feed in priority order: workspace first,
|
||||
// user second. The user-side record then gets shadowed.
|
||||
assert!(reg.merge_skill(ws_record).is_none());
|
||||
let shadow = reg.merge_skill(user_record).expect("user should shadow");
|
||||
assert_eq!(shadow.kept_path, ws_path);
|
||||
assert!(matches!(shadow.kept_source, WorkflowSource::WorkspaceSkill { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shadow_message_is_human_readable() {
|
||||
let s = ShadowedSkill {
|
||||
slug: Slug::parse("x").unwrap(),
|
||||
kept_source: WorkflowSource::WorkspaceWorkflow,
|
||||
kept_path: std::path::PathBuf::from("/ws/.insomnia/memory/workflow/x.md"),
|
||||
shadowed_source: WorkflowSource::UserSkill {
|
||||
dir: std::path::PathBuf::from("/user/skills"),
|
||||
},
|
||||
shadowed_path: std::path::PathBuf::from("/user/skills/x/SKILL.md"),
|
||||
};
|
||||
let msg = s.message();
|
||||
assert!(msg.contains("/x"));
|
||||
assert!(msg.contains("workspace workflow"));
|
||||
assert!(msg.contains("user skill"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resident_description_cap_is_enforced() {
|
||||
let (dir, layout) = setup();
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ use session_store::{EntryHash, PodScopeSnapshot, SessionId, SessionStartState, S
|
|||
use tracing::{info, warn};
|
||||
|
||||
use manifest::{
|
||||
PodManifest, PodManifestConfig, ResolveError, Scope, ScopeConfig, ScopeError, ScopeRule,
|
||||
Permission, PodManifest, PodManifestConfig, ResolveError, Scope, ScopeConfig, ScopeError, ScopeRule,
|
||||
SharedScope, WorkerManifest,
|
||||
};
|
||||
|
||||
|
|
@ -2225,7 +2225,8 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
store: St,
|
||||
loader: PromptLoader,
|
||||
) -> Result<Self, PodError> {
|
||||
let common = prepare_pod_common(&manifest, &loader, /* parse_template */ true)?;
|
||||
let mut common = prepare_pod_common(&manifest, &loader, /* parse_template */ true)?;
|
||||
let skill_shadows = std::mem::take(&mut common.skill_shadows);
|
||||
|
||||
// Session creation is deferred to the first run (see
|
||||
// `ensure_session_head`) so the SessionStart entry can capture
|
||||
|
|
@ -2291,6 +2292,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
user_segments: Vec::new(),
|
||||
};
|
||||
pod.apply_prune_from_manifest();
|
||||
drain_skill_shadows(&pod, skill_shadows);
|
||||
Ok(pod)
|
||||
}
|
||||
|
||||
|
|
@ -2308,7 +2310,8 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
loader: PromptLoader,
|
||||
callback_socket: PathBuf,
|
||||
) -> Result<Self, PodError> {
|
||||
let common = prepare_pod_common(&manifest, &loader, /* parse_template */ true)?;
|
||||
let mut common = prepare_pod_common(&manifest, &loader, /* parse_template */ true)?;
|
||||
let skill_shadows = std::mem::take(&mut common.skill_shadows);
|
||||
|
||||
let session_id = session_store::new_session_id();
|
||||
let scope_allocation = pod_registry::adopt_allocation(
|
||||
|
|
@ -2359,6 +2362,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
user_segments: Vec::new(),
|
||||
};
|
||||
pod.apply_prune_from_manifest();
|
||||
drain_skill_shadows(&pod, skill_shadows);
|
||||
Ok(pod)
|
||||
}
|
||||
|
||||
|
|
@ -2395,7 +2399,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
.clone()
|
||||
.ok_or(PodError::SessionScopeMissing { session_id })?;
|
||||
|
||||
let common = prepare_pod_common_with_scope(
|
||||
let mut common = prepare_pod_common_with_scope(
|
||||
&manifest,
|
||||
&loader,
|
||||
/* parse_template */ false,
|
||||
|
|
@ -2404,6 +2408,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
deny: scope_snapshot.deny,
|
||||
},
|
||||
)?;
|
||||
let skill_shadows = std::mem::take(&mut common.skill_shadows);
|
||||
|
||||
// Atomic: register_pod inside install_top_level rejects when
|
||||
// another live allocation already holds `session_id`. Wrapping
|
||||
|
|
@ -2493,6 +2498,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
user_segments: state.user_segments,
|
||||
};
|
||||
pod.apply_prune_from_manifest();
|
||||
drain_skill_shadows(&pod, skill_shadows);
|
||||
Ok(pod)
|
||||
}
|
||||
|
||||
|
|
@ -2734,6 +2740,11 @@ struct PodCommon {
|
|||
workflow_registry: memory::WorkflowRegistry,
|
||||
memory_layout: Option<memory::WorkspaceLayout>,
|
||||
system_prompt_template: Option<SystemPromptTemplate>,
|
||||
/// SKILL.md shadow events surfaced during workflow-registry build.
|
||||
/// The Pod constructor drains these into the notify buffer right
|
||||
/// after the Pod is materialised so the first LLM request observes
|
||||
/// any skill ↔ workflow collisions.
|
||||
skill_shadows: Vec<memory::ShadowedSkill>,
|
||||
}
|
||||
|
||||
/// Resolve pwd / scope / LLM client / prompt catalog from a validated
|
||||
|
|
@ -2784,10 +2795,11 @@ fn prepare_pod_common_from_scope(
|
|||
.memory
|
||||
.as_ref()
|
||||
.map(|mem| memory::WorkspaceLayout::resolve(mem, &pwd));
|
||||
let workflow_registry = match memory_layout.as_ref() {
|
||||
let mut workflow_registry = match memory_layout.as_ref() {
|
||||
Some(layout) => memory::load_workflows(layout).map_err(PodError::WorkflowLoad)?,
|
||||
None => memory::WorkflowRegistry::empty(),
|
||||
};
|
||||
let skill_shadows = ingest_skills(&mut workflow_registry, manifest);
|
||||
|
||||
let system_prompt_template = if parse_template {
|
||||
Some(
|
||||
|
|
@ -2806,24 +2818,108 @@ fn prepare_pod_common_from_scope(
|
|||
workflow_registry,
|
||||
memory_layout,
|
||||
system_prompt_template,
|
||||
skill_shadows,
|
||||
})
|
||||
}
|
||||
|
||||
/// Ingest external SKILL.md sources into the workflow registry.
|
||||
///
|
||||
/// Sources are tried in descending priority so the registry's "first
|
||||
/// insert wins" semantics line up with the spec's collision order:
|
||||
/// 1. workspace-authored Workflows (already in `registry` from
|
||||
/// [`memory::load_workflows`])
|
||||
/// 2. workspace skills declared by the manifest's `[skills] directories`
|
||||
/// 3. user skills under `$config_dir/skills/`
|
||||
///
|
||||
/// Returns the list of shadowed-skill events the Pod should later push
|
||||
/// onto its notification buffer.
|
||||
fn ingest_skills(
|
||||
registry: &mut memory::WorkflowRegistry,
|
||||
manifest: &PodManifest,
|
||||
) -> Vec<memory::ShadowedSkill> {
|
||||
let mut shadows = Vec::new();
|
||||
if let Some(skills_cfg) = manifest.skills.as_ref() {
|
||||
for dir in &skills_cfg.directories {
|
||||
for skill in memory::load_skills_from_dir(dir) {
|
||||
let source = memory::WorkflowSource::WorkspaceSkill { dir: dir.clone() };
|
||||
let record = skill.into_workflow_record(source);
|
||||
if let Some(shadow) = registry.merge_skill(record) {
|
||||
shadows.push(shadow);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
if let Some(user_dir) = manifest::paths::user_skills_dir() {
|
||||
for skill in memory::load_skills_from_dir(&user_dir) {
|
||||
let source = memory::WorkflowSource::UserSkill {
|
||||
dir: user_dir.clone(),
|
||||
};
|
||||
let record = skill.into_workflow_record(source);
|
||||
if let Some(shadow) = registry.merge_skill(record) {
|
||||
shadows.push(shadow);
|
||||
}
|
||||
}
|
||||
}
|
||||
shadows
|
||||
}
|
||||
|
||||
/// Drain skill-ingest shadow events into the Pod's notify buffer so the
|
||||
/// first LLM request renders them as system-message attachments.
|
||||
fn drain_skill_shadows<C, S>(pod: &Pod<C, S>, shadows: Vec<memory::ShadowedSkill>)
|
||||
where
|
||||
C: LlmClient,
|
||||
S: Store,
|
||||
{
|
||||
for shadow in shadows {
|
||||
pod.push_notify(format!("[Skill shadowed] {}", shadow.message()));
|
||||
}
|
||||
}
|
||||
|
||||
/// Build the Pod's runtime [`Scope`] from the manifest, layering the
|
||||
/// memory subsystem's deny-write rules on top when `[memory]` is
|
||||
/// present. The deny rules cap generic CRUD tools so they cannot
|
||||
/// touch `<workspace>/memory/` or `<workspace>/knowledge/` while the
|
||||
/// memory tools (registered separately) bypass `ScopedFs` and write
|
||||
/// through `std::fs` directly.
|
||||
/// present, and read-allow rules for any external Agent Skills
|
||||
/// directories ingested. The deny rules cap generic CRUD tools so they
|
||||
/// cannot touch `<workspace>/memory/` or `<workspace>/knowledge/` while
|
||||
/// the memory tools (registered separately) bypass `ScopedFs` and write
|
||||
/// through `std::fs` directly. Skill directories are added at
|
||||
/// `Permission::Read` so the agent can `Read` `scripts/` / `references/`
|
||||
/// / `assets/` referenced by the Workflow body.
|
||||
fn build_scope_with_memory(manifest: &PodManifest, pwd: &Path) -> Result<Scope, PodError> {
|
||||
let mut scope_config = manifest.scope.clone();
|
||||
if let Some(mem) = manifest.memory.as_ref() {
|
||||
let layout = memory::WorkspaceLayout::resolve(mem, pwd);
|
||||
scope_config.deny.extend(memory::deny_write_rules(&layout));
|
||||
}
|
||||
scope_config.allow.extend(skill_dir_read_rules(manifest));
|
||||
Scope::from_config(&scope_config).map_err(PodError::Scope)
|
||||
}
|
||||
|
||||
/// Allow-rules granting `Read` access to every skill directory the Pod
|
||||
/// will ingest: the `[skills] directories` from the manifest plus the
|
||||
/// user-level `$config_dir/skills/`. Returned rules are recursive so
|
||||
/// the entire skill bundle (`SKILL.md` + `scripts/` + `references/` +
|
||||
/// `assets/`) is readable.
|
||||
fn skill_dir_read_rules(manifest: &PodManifest) -> Vec<ScopeRule> {
|
||||
let mut rules = Vec::new();
|
||||
if let Some(skills_cfg) = manifest.skills.as_ref() {
|
||||
for dir in &skills_cfg.directories {
|
||||
rules.push(ScopeRule {
|
||||
target: dir.clone(),
|
||||
permission: Permission::Read,
|
||||
recursive: true,
|
||||
});
|
||||
}
|
||||
}
|
||||
if let Some(user_dir) = manifest::paths::user_skills_dir() {
|
||||
rules.push(ScopeRule {
|
||||
target: user_dir,
|
||||
permission: Permission::Read,
|
||||
recursive: true,
|
||||
});
|
||||
}
|
||||
rules
|
||||
}
|
||||
|
||||
/// Snapshot the process's current working directory as the Pod's pwd,
|
||||
/// canonicalising symlinks and any `.`/`..` components. The Pod keeps
|
||||
/// this value for its lifetime; changes to the process-wide cwd after
|
||||
|
|
@ -2911,4 +3007,82 @@ mod build_summary_prompt_tests {
|
|||
let prompt = build_summary_prompt(&items);
|
||||
assert_eq!(prompt, "[User] fix the bug\n\n[Assistant] done");
|
||||
}
|
||||
|
||||
fn minimal_manifest_with_skills(dirs: Vec<PathBuf>) -> PodManifest {
|
||||
// Construct the smallest possible PodManifest that resolves; only
|
||||
// the `skills` field matters for `skill_dir_read_rules`.
|
||||
let toml_str = r#"
|
||||
[pod]
|
||||
name = "x"
|
||||
|
||||
[model]
|
||||
scheme = "anthropic"
|
||||
model_id = "claude-sonnet-4-20250514"
|
||||
|
||||
[worker]
|
||||
|
||||
[[scope.allow]]
|
||||
target = "/abs/scope"
|
||||
permission = "write"
|
||||
"#;
|
||||
let mut manifest = PodManifest::from_toml(toml_str).unwrap();
|
||||
if !dirs.is_empty() {
|
||||
manifest.skills = Some(manifest::SkillsConfig { directories: dirs });
|
||||
}
|
||||
manifest
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skill_dir_read_rules_lists_workspace_skill_directories() {
|
||||
let manifest = minimal_manifest_with_skills(vec![
|
||||
PathBuf::from("/abs/skills-a"),
|
||||
PathBuf::from("/abs/skills-b"),
|
||||
]);
|
||||
let rules = skill_dir_read_rules(&manifest);
|
||||
let workspace_rules: Vec<_> = rules
|
||||
.iter()
|
||||
.filter(|r| {
|
||||
r.target == PathBuf::from("/abs/skills-a")
|
||||
|| r.target == PathBuf::from("/abs/skills-b")
|
||||
})
|
||||
.collect();
|
||||
assert_eq!(workspace_rules.len(), 2);
|
||||
for rule in &workspace_rules {
|
||||
assert_eq!(rule.permission, Permission::Read);
|
||||
assert!(rule.recursive);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skill_dir_read_rules_ignores_missing_skills_section() {
|
||||
let manifest = minimal_manifest_with_skills(vec![]);
|
||||
let rules = skill_dir_read_rules(&manifest);
|
||||
// Whatever rules we get must all be Read+recursive (the user
|
||||
// skills directory may or may not resolve depending on env).
|
||||
for rule in &rules {
|
||||
assert_eq!(rule.permission, Permission::Read);
|
||||
assert!(rule.recursive);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ingest_skills_loads_from_workspace_directories() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let skills_root = dir.path().join("skills");
|
||||
std::fs::create_dir_all(skills_root.join("alpha")).unwrap();
|
||||
std::fs::write(
|
||||
skills_root.join("alpha").join("SKILL.md"),
|
||||
"---\nname: alpha\ndescription: Alpha skill\n---\nbody\n",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let manifest = minimal_manifest_with_skills(vec![skills_root.clone()]);
|
||||
let mut registry = memory::WorkflowRegistry::empty();
|
||||
let shadows = ingest_skills(&mut registry, &manifest);
|
||||
|
||||
// workspace skill `alpha` should be registered (no collision).
|
||||
assert!(registry.get(&memory::Slug::parse("alpha").unwrap()).is_some());
|
||||
// No workflow exists to shadow `alpha`, so no shadow event for it.
|
||||
assert!(shadows.iter().all(|s| s.slug.as_str() != "alpha"));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -118,3 +118,9 @@ skill ディレクトリ全体(`SKILL.md` 本体だけでなく `scripts/` `re
|
|||
- 検証 CLI: https://github.com/agentskills/agentskills/tree/main/skills-ref
|
||||
- 前提: `tickets/workflow.md`
|
||||
- 関連: `tickets/permission-extension-point.md`(`allowed-tools` 実効化の受け皿)、`docs/plan/workflow.md`、`docs/plan/memory.md`、`docs/ref/memory-systems.md` §Skill Library
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./agent-skills.review.md](./agent-skills.review.md)
|
||||
- 日付: 2026-05-04
|
||||
|
|
|
|||
80
tickets/agent-skills.review.md
Normal file
80
tickets/agent-skills.review.md
Normal file
|
|
@ -0,0 +1,80 @@
|
|||
# Review: Agent Skills を Workflow として ingest
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
### 完了条件マッピング
|
||||
|
||||
| 条件 | 状態 | 根拠 |
|
||||
|---|---|---|
|
||||
| `$user/skills/` 配下の `SKILL.md` が Workflow として登録され `/<name>` で呼び出せる | 満 | `crates/pod/src/pod.rs:2849-2862` で `manifest::paths::user_skills_dir()` を `load_skills_from_dir` → `merge_skill`。生成 `WorkflowRecord` は `user_invocable: true`/`model_invokation: true` (`crates/memory/src/skill.rs:73-84`) なので既存 resolver と `list_user_invocable` がそのまま拾う |
|
||||
| manifest `[skills] directories` 指定 workspace で追加 ingest、未指定なら 0 件 | 満 | `crates/pod/src/pod.rs:2840-2847` が `manifest.skills.as_ref()` 配下のみ走査。未指定なら `if let Some(...)` を抜けるだけで何も読まない |
|
||||
| 内製 Workflow と同 slug の skill は内製優先で shadow し Notification を発行 | 満 | `WorkflowRegistry::merge_skill` (`crates/memory/src/workflow.rs:146-158`) が既存 record 保持、`ShadowedSkill` を返す。`drain_skill_shadows` (`crates/pod/src/pod.rs:2868-2876`) で `push_notify` |
|
||||
| skill ディレクトリ全体 (`SKILL.md`/`scripts/`/`references/`/`assets/`) が scope readable | 満 | `skill_dir_read_rules` (`crates/pod/src/pod.rs:2902-2922`) が `Permission::Read` + `recursive: true` で `build_scope_with_memory` の `scope_config.allow.extend` に乗る |
|
||||
| frontmatter 違反は warn でスキップ、他 skill / Pod 起動に影響しない | 満 | `load_skills_from_dir` (`crates/memory/src/skill.rs:202-248`) が `parse_skill_md` のエラーを `tracing::warn!` で握り潰し、結果を Vec に積む。Pod 構築フローは `Result` を返さない |
|
||||
| 単体テスト: frontmatter 検証 / Workflow マッピング / 衝突解決 / manifest 未指定時 workspace skip | 概ね満 (1 点 nit) | 後述「テストカバレッジ」参照 |
|
||||
|
||||
### 方針マッピング
|
||||
|
||||
- ロードソースの優先順 (内製 → workspace → user) — `ingest_skills` (`crates/pod/src/pod.rs:2836-2867`) で実現。registry にあらかじめ載った内製 Workflow が `merge_skill` で常に勝ち、workspace を user より先に feed することで spec 通りの順位が出る
|
||||
- `model_invokation: true` / `user_invocable: true` 固定、`requires` 空 — `SkillRecord::into_workflow_record` (`crates/memory/src/skill.rs:73-84`) で固定
|
||||
- `allowed-tools` は warn で受け流し — `crates/memory/src/skill.rs:156-161` で `tracing::warn!`。Permission 層の受け皿契約は `tickets/permission-extension-point.md` 側に追記済み
|
||||
- 検証は内製 Workflow より緩く個別 skill 単位 skip — `load_skills_from_dir` の per-entry `match` で `tracing::warn!` のみ。Pod 構築は止まらない
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
### 既存パターンとの整合
|
||||
|
||||
- **manifest/scope 拡張**: `SkillsConfig` の `merge` は他の `merge_option` と同じ pattern (`crates/manifest/src/config.rs:215-220`)。`resolve_paths` は既存の `auth.path` などと同様 `join_if_relative(base, dir)` を使い、`TryFrom<PodManifestConfig>` で `ensure_absolute` を呼ぶ三段構え。ScopeConfig の他 field と同じ流儀で破綻なし
|
||||
- **memory crate**: `skill.rs` を新ファイルに切り出し、`workflow.rs` には `WorkflowSource` / `ShadowedSkill` / `merge_skill` のみを足す。`WorkflowRecord` 自身に skill 起源固有の field を生やしていないので、`load_workflows` 側のエラー型 (`WorkflowLoadError`) はそのまま。SKILL 用エラー型は `SkillParseError` に分離 — 内製 Workflow と外部 skill で「壊れたら hard error / 壊れたら warn skip」の温度差が型レベルで表現されている
|
||||
- **scope union pattern**: `build_scope_with_memory` が `deny_write_rules`/`skill_dir_read_rules` を `extend` する構造。`scope::deny_write_rules` の参照モデルと完全に対称
|
||||
- **`paths::user_skills_dir`**: `user_prompts_dir` (`crates/manifest/src/paths.rs:78-81`) と並列に並ぶ命名・実装。違和感なし
|
||||
- **Cargo**: `serde_yaml` / `tempfile` / `tracing` / `thiserror` は既に memory の依存にあり (`crates/memory/Cargo.toml:16-22`)。新規追加は無く、Cargo 系のガイドライン (cargo add 縛り) には抵触しない
|
||||
|
||||
### 範囲外項目への侵入チェック
|
||||
|
||||
- `allowed-tools` 実効化: 行わず warn 止まり。`tickets/permission-extension-point.md` 側に受け皿契約を追記しているのは妥当な情報の遺し方で、本チケットのスコープ外実装には踏み込んでいない
|
||||
- `scripts/` 自動実行: しない。Read 経由の閲覧のみで、Bash 経由実行は Permission 層待ち
|
||||
- ビルトイン skill / 自動生成 / 間依存: いずれも実装無し
|
||||
- skill 用 protocol message / event 種別の追加: 無し。`Method::Notify` の既存ルートを再利用
|
||||
|
||||
### 不必要な抽象 / 過剰な API のチェック
|
||||
|
||||
- `WorkflowSource::label()` は `ShadowedSkill::message()` でしか使われない (`crates/memory/src/workflow.rs:42-49`)。public で晒されているが UI/log の他の経路で再利用される含みはあるので許容範囲
|
||||
- `SkillRecord` を一旦経由してから `into_workflow_record` で `WorkflowRecord` に投影する 2 段構造は、当面 `SkillRecord` の独自情報 (allowed_tools 等) を持たせる場が無いので冗長気味。ただし「Permission 層が来たときに skill 固有 metadata の保持先になる」という拡張ポイントとして見れば、無駄ではない (受け皿チケット側の文面とも整合)
|
||||
- `WorkflowRegistry::merge_skill` の「呼び出し側に降順 priority feed を強制し registry は再ランクしない」契約は doc comment で明記されており、ingest 側 `ingest_skills` でその順序を遵守。registry 側で抽象化を増やすより素直
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
|
||||
なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- **`restore_from_manifest` でも shadow 通知を emit する設計の説明不足** — `crates/pod/src/pod.rs:2501` で resume 時にも `drain_skill_shadows` を呼ぶ。実装ロジックとしては「registry を再構築する以上、衝突は再評価される」という意味で一貫しているが、ユーザーから見ると同じ session が一度落ちて起動するたびに `[Skill shadowed]` が再注入される。`drain_skill_shadows` か `restore_from_manifest` 周辺に「resume 時にも (重複で) 出すのは仕様」と一行コメントを残しておくと、後で疑問が再燃しない
|
||||
- **`WorkflowRegistry::merge_skill` の事前条件依存性** — 「降順 priority feed を呼び出し側が守る」契約は doc comment にあるが、debug assertion (例えば 2 回目以降の skill record が既存 skill record を shadow しようとしたとき、`ws/user` の順序が逆だったら `debug_assert!`) は無い。将来 `ingest_skills` 以外の caller が増えたときに気付きにくいので、follow-up で防衛コードを足す余地あり
|
||||
- **`Slug::parse` が agent-skills 仕様より厳しい** — 仕様は `[a-z0-9-]+` だが `Slug` は先頭末尾の `-` と `--` を禁止。今 `[a-z0-9-]+` 厳密準拠の skill は実用上ほぼ無く、内製 Workflow の slug ルールと一致させる方が合理的なので、現状の厳しめ運用で良い。ただしユーザー観点での挙動 (warn で skip される) を docs/plan で明示しておく価値はある
|
||||
- **scope.allow が常に user_skills_dir を含む** — `skill_dir_read_rules` (`crates/pod/src/pod.rs:2902-2922`) は manifest 側に skills 設定が無くとも `~/.config/insomnia/skills` を allow に追加する。`resolve_path` は存在しない path を最寄りの祖先に anchor して許容するので技術的には壊れないが、「ユーザーがそのディレクトリを意図せず作っただけで Pod の scope に readable として乗る」という挙動は ticket の背景 (デフォルトで有効) に沿っており妥当。ただし「ユーザーが skills 機能を完全に opt-out したい」場合の手段が現状無いので、Phase 2 で `[skills] enabled = false` 等の opt-out を検討する余地あり
|
||||
- **`pod.rs` のテスト 3 件が `skill_dir_read_rules`/`ingest_skills` のホワイトボックステスト** — 衝突発生時の `shadow` Notify 経路 (`drain_skill_shadows` が `push_notify` を確実に呼ぶこと) を end-to-end で検証するテストはまだ無い。`memory/src/workflow.rs` 側で `merge_skill` の戻り値検証はあるので致命的ではないが、Pod 構築から Notification 到達までの統合的な確認は将来 follow-up に回せる
|
||||
|
||||
### Nits
|
||||
|
||||
- `crates/memory/src/skill.rs:316-326` (`invalid_slug_name_is_error`): `BAD-Caps` という入力は dir 名としても禁止されている (大文字を含むため `Slug::parse` が rejected する)。コメントで「`InvalidName` か `NameDirMismatch` のどちらかになる」と書いてあるが、実際にどちらが先に発火するかをテストで明示しておくと、validation 順序を変えたときに気付きやすい
|
||||
- `crates/pod/src/pod.rs:14-16`: `Permission` を import 行に追加した整形で行が 100 文字を超えそうに見える (rustfmt は通っているはずなので問題ないが、`use manifest::{}` ブロックがリフォーマットしたほうが読みやすい)
|
||||
- `[Skill shadowed]` 文言は agent 側に渡る system message としては短くて十分だが、今後類似のシステム由来 notify が増えたときに prefix の語彙統一 (`[Memory shadowed]` / `[Knowledge ...]` 等) を docs/plan で揃える価値あり
|
||||
|
||||
## テストカバレッジ
|
||||
|
||||
ticket の完了条件にある「frontmatter 検証、Workflow へのマッピング、衝突解決、manifest 未指定時の workspace skip」を順に確認:
|
||||
|
||||
- frontmatter 検証: `parses_minimal_skill` / `name_dir_mismatch_is_error` / `invalid_slug_name_is_error` / `empty_description_is_error` / `description_at_cap_is_accepted` / `description_over_cap_is_error` / `missing_frontmatter_is_error` / `extra_frontmatter_fields_are_kept` (`crates/memory/src/skill.rs`) — 過不足なし
|
||||
- Workflow マッピング: `into_workflow_record_uses_skill_defaults` (`crates/memory/src/skill.rs:415-435`) で `model_invokation`/`user_invocable`/`requires`/`source` を確認
|
||||
- 衝突解決:
|
||||
- `merge_skill_inserts_when_no_collision` / `merge_skill_shadows_existing_workflow` / `merge_skill_priority_workspace_over_user` / `shadow_message_is_human_readable` (`crates/memory/src/workflow.rs:396-490`)
|
||||
- manifest 未指定時の workspace skip: 直接の単体テストは無いが、`ingest_skills_loads_from_workspace_directories` (`crates/pod/src/pod.rs:3068-3087`) で `manifest.skills = Some(...)` のケースを cover、`skill_dir_read_rules_ignores_missing_skills_section` (`crates/pod/src/pod.rs:3057-3066`) が Skill 設定無しでの allow rules を検査。`ingest_skills` の構造的に未指定なら for ループに入らない事は自明だが、念のため「未指定 manifest で `ingest_skills` が空 Vec を返す」テストを 1 件足すとカバレッジが完結する (non-blocking)
|
||||
|
||||
`cargo test --workspace --no-fail-fast` を実行: 全 crate で 0 failed を確認。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve** — チケットの完了条件は要件・テストカバレッジともすべて満たされており、既存の manifest cascade / scope union / push_notify / WorkflowRegistry の各パターンに沿って実装されている。範囲外項目 (`allowed-tools` 実効化、`scripts/` 自動実行、ビルトイン skill 等) には踏み込まず、受け皿契約の追記も適切。Non-blocking の follow-up は将来チケットで巻き取って良い性質のもの。
|
||||
|
|
@ -49,6 +49,19 @@ action = "allow"
|
|||
2. **deny/allow の実装**(ツール実装時): PreToolCall Hook でパターン評価
|
||||
3. **ask の実装**(Protocol 拡張時): Method/Event に Permission 関連メッセージを追加
|
||||
|
||||
## 受け皿になる外部仕様
|
||||
|
||||
### Agent Skills `allowed-tools`
|
||||
|
||||
`tickets/agent-skills.md` で ingest した SKILL.md の frontmatter には agent-skills 仕様の experimental field である `allowed-tools` (例: `["Read", "Bash"]`) が含まれる場合がある。`crates/memory/src/skill.rs::parse_skill_md` 時点では `tracing::warn!` で受け流しているだけで、実効化していない。
|
||||
|
||||
本チケットの Permission 層が固まった時点で、Skill 由来 Workflow を実行中のみ当該 skill の `allowed-tools` リストに含まれるツールしか走れない形で反映する。スコープは「Workflow 実行中」相当 (Workflow の system message が context に乗っているターン) に限定する想定。skill 単位で local な permission 集合を持つので、グローバルな `[[permission]]` ルールとは独立に評価する。
|
||||
|
||||
実装上の足がかり:
|
||||
- `WorkflowRecord` の出所は `WorkflowSource::WorkspaceSkill { dir }` / `UserSkill { dir }` で識別済み (`crates/memory/src/workflow.rs`)
|
||||
- 受け皿実装時に `SkillFrontmatter::allowed_tools` の保持先を `WorkflowRecord` に伸ばすか、別の SkillRecord registry を持つかは本チケット内で決める
|
||||
- 現状の `tracing::warn!` は受け皿実装と同時に消す
|
||||
|
||||
## 依存チケット
|
||||
|
||||
- ~~[remove-hook-module.md](remove-hook-module.md)~~ — 完了。PreToolCall は Pod 層の `hook::Hook<PreToolCall>` として利用可能
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user