diff --git a/crates/manifest/src/config.rs b/crates/manifest/src/config.rs index 8469c49b..58809d84 100644 --- a/crates/manifest/src/config.rs +++ b/crates/manifest/src/config.rs @@ -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, + /// External Agent Skills directories. See [`crate::SkillsConfig`]. + #[serde(default)] + pub skills: Option, } #[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 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 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 { diff --git a/crates/manifest/src/lib.rs b/crates/manifest/src/lib.rs index 09c290ce..3ab839fa 100644 --- a/crates/manifest/src/lib.rs +++ b/crates/manifest/src/lib.rs @@ -44,6 +44,30 @@ pub struct PodManifest { /// memory tools registered. #[serde(default)] pub memory: Option, + /// 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 `/SKILL.md` skill bundles). Paths + /// are resolved against the manifest's base directory like other + /// path fields. + #[serde(default)] + pub skills: Option, +} + +/// 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 + /// `/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, } /// Memory subsystem configuration. Presence in the manifest enables diff --git a/crates/manifest/src/paths.rs b/crates/manifest/src/paths.rs index 96f9873f..9e16693f 100644 --- a/crates/manifest/src/paths.rs +++ b/crates/manifest/src/paths.rs @@ -79,6 +79,12 @@ pub fn user_prompts_dir() -> Option { Some(config_dir()?.join("prompts")) } +/// `/skills/` — user-level Agent Skills ライブラリ。 +/// 配下は `/SKILL.md` の集合として読まれる。 +pub fn user_skills_dir() -> Option { + Some(config_dir()?.join("skills")) +} + /// `/prompts.toml` — user prompt pack。 pub fn user_pack_file() -> Option { Some(config_dir()?.join("prompts.toml")) diff --git a/crates/memory/src/lib.rs b/crates/memory/src/lib.rs index b34475d6..9fac51e5 100644 --- a/crates/memory/src/lib.rs +++ b/crates/memory/src/lib.rs @@ -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; diff --git a/crates/memory/src/skill.rs b/crates/memory/src/skill.rs new file mode 100644 index 00000000..97941685 --- /dev/null +++ b/crates/memory/src/skill.rs @@ -0,0 +1,447 @@ +//! Agent Skills (`SKILL.md`) parser. +//! +//! Skills follow the [agentskills.io](https://agentskills.io/specification) +//! spec: a directory `//` 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 `/` 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/.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, + #[serde(default)] + pub compatibility: Option, + #[serde(default)] + pub metadata: Option, + #[serde(default, rename = "allowed-tools")] + pub allowed_tools: Option, +} + +/// 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 `/`, 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 { + 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 `//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 { + 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 = 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::(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"]); + } +} diff --git a/crates/memory/src/workflow.rs b/crates/memory/src/workflow.rs index c9adc66f..44898997 100644 --- a/crates/memory/src/workflow.rs +++ b/crates/memory/src/workflow.rs @@ -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 { + /// `/.insomnia/memory/workflow/.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 + /// `/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 { + 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 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(); diff --git a/crates/pod/src/pod.rs b/crates/pod/src/pod.rs index f6d6143f..36c1a475 100644 --- a/crates/pod/src/pod.rs +++ b/crates/pod/src/pod.rs @@ -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 Pod, St> { store: St, loader: PromptLoader, ) -> Result { - 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 Pod, St> { user_segments: Vec::new(), }; pod.apply_prune_from_manifest(); + drain_skill_shadows(&pod, skill_shadows); Ok(pod) } @@ -2308,7 +2310,8 @@ impl Pod, St> { loader: PromptLoader, callback_socket: PathBuf, ) -> Result { - 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 Pod, St> { user_segments: Vec::new(), }; pod.apply_prune_from_manifest(); + drain_skill_shadows(&pod, skill_shadows); Ok(pod) } @@ -2395,7 +2399,7 @@ impl Pod, 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 Pod, 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 Pod, 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, system_prompt_template: Option, + /// 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, } /// 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 { + 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(pod: &Pod, shadows: Vec) +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 `/memory/` or `/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 `/memory/` or `/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 { 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 { + 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) -> 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")); + } } diff --git a/tickets/agent-skills.md b/tickets/agent-skills.md index 05fa8c6d..67f86b22 100644 --- a/tickets/agent-skills.md +++ b/tickets/agent-skills.md @@ -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 diff --git a/tickets/agent-skills.review.md b/tickets/agent-skills.review.md new file mode 100644 index 00000000..6a998eb7 --- /dev/null +++ b/tickets/agent-skills.review.md @@ -0,0 +1,80 @@ +# Review: Agent Skills を Workflow として ingest + +## 前提・要件の確認 + +### 完了条件マッピング + +| 条件 | 状態 | 根拠 | +|---|---|---| +| `$user/skills/` 配下の `SKILL.md` が Workflow として登録され `/` で呼び出せる | 満 | `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` で `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 は将来チケットで巻き取って良い性質のもの。 diff --git a/tickets/permission-extension-point.md b/tickets/permission-extension-point.md index d8b4ab8e..ab401fc1 100644 --- a/tickets/permission-extension-point.md +++ b/tickets/permission-extension-point.md @@ -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` として利用可能