From 85fe1a094c9f99b458181a1379556541906d8c6b Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 5 May 2026 13:54:02 +0900 Subject: [PATCH] =?UTF-8?q?update:=20Agent=20skills=E5=AE=9F=E8=A3=85?= =?UTF-8?q?=E3=81=AE=E3=83=AC=E3=83=93=E3=83=A5=E3=83=BC=E3=83=BB=E5=AF=BE?= =?UTF-8?q?=E5=BF=9C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/manifest/src/lib.rs | 22 ++--- crates/manifest/src/paths.rs | 6 -- crates/memory/src/skill.rs | 4 +- crates/memory/src/workflow.rs | 78 ++++++++-------- crates/pod/src/pod.rs | 88 +++++++----------- tickets/agent-skills.md | 41 ++++---- tickets/agent-skills.review.md | 129 ++++++++++++++++---------- tickets/permission-extension-point.md | 2 +- 8 files changed, 188 insertions(+), 182 deletions(-) diff --git a/crates/manifest/src/lib.rs b/crates/manifest/src/lib.rs index 3ab839fa..69f74db9 100644 --- a/crates/manifest/src/lib.rs +++ b/crates/manifest/src/lib.rs @@ -45,21 +45,21 @@ pub struct PodManifest { #[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. + /// Workflows. 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. Absent ⇒ no skills loaded; + /// there is no implicit `$config_dir/skills/` or builtin probe. #[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. +/// External Agent Skills (`SKILL.md`) ingest configuration. Skills are +/// loaded *only* from the directories listed here — there is no +/// implicit `$config_dir/skills/` or builtin probe. Cascade-merged +/// across manifest layers, so a user-level manifest can declare a +/// shared skill root once while a project manifest adds its own +/// `.claude/skills/` / `.cursor/skills/` paths on top. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct SkillsConfig { /// Skills *roots*. Children of each root must be individual diff --git a/crates/manifest/src/paths.rs b/crates/manifest/src/paths.rs index 9e16693f..96f9873f 100644 --- a/crates/manifest/src/paths.rs +++ b/crates/manifest/src/paths.rs @@ -79,12 +79,6 @@ 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/skill.rs b/crates/memory/src/skill.rs index 97941685..113ad965 100644 --- a/crates/memory/src/skill.rs +++ b/crates/memory/src/skill.rs @@ -422,7 +422,7 @@ mod tests { "Steps\n", ); let record = parse_skill_md(&path).unwrap(); - let wf = record.into_workflow_record(WorkflowSource::UserSkill { + let wf = record.into_workflow_record(WorkflowSource::Skill { dir: dir.path().to_path_buf(), }); assert_eq!(wf.slug.as_str(), "x"); @@ -431,7 +431,7 @@ mod tests { assert!(wf.user_invocable); assert!(wf.requires.is_empty()); assert_eq!(wf.body, "Steps\n"); - assert!(matches!(wf.source, WorkflowSource::UserSkill { .. })); + assert!(matches!(wf.source, WorkflowSource::Skill { .. })); } #[test] diff --git a/crates/memory/src/workflow.rs b/crates/memory/src/workflow.rs index 44898997..da1ea4ab 100644 --- a/crates/memory/src/workflow.rs +++ b/crates/memory/src/workflow.rs @@ -23,19 +23,16 @@ 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. +/// win over external 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 + /// 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 }, + Skill { dir: PathBuf }, } impl WorkflowSource { @@ -43,8 +40,7 @@ impl WorkflowSource { pub fn label(&self) -> &'static str { match self { Self::WorkspaceWorkflow => "workspace workflow", - Self::WorkspaceSkill { .. } => "workspace skill", - Self::UserSkill { .. } => "user skill", + Self::Skill { .. } => "skill", } } } @@ -138,11 +134,11 @@ impl WorkflowRegistry { } /// Insert a skill-derived record. If an existing record (internal - /// Workflow or higher-priority skill) already owns the slug, the + /// Workflow or earlier-fed 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. + /// collision is returned. Callers feed records in priority order + /// (highest first); the registry is "first-insert wins" and does + /// not re-rank. pub fn merge_skill(&mut self, record: WorkflowRecord) -> Option { if let Some(existing) = self.records.get(&record.slug) { return Some(ShadowedSkill { @@ -386,7 +382,7 @@ mod tests { requires: Vec::new(), body: format!("body for {slug}"), path: path.to_path_buf(), - source: WorkflowSource::WorkspaceSkill { + source: WorkflowSource::Skill { dir: path.parent().unwrap().parent().unwrap().to_path_buf(), }, } @@ -417,14 +413,14 @@ mod tests { requires: Vec::new(), body: "skill body".into(), path: skill_path.clone(), - source: WorkflowSource::UserSkill { + source: WorkflowSource::Skill { 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 { .. })); + assert!(matches!(shadow.shadowed_source, WorkflowSource::Skill { .. })); // The kept record is still the workspace workflow. let kept = reg.get(&Slug::parse("shared").unwrap()).unwrap(); assert!(matches!(kept.source, WorkflowSource::WorkspaceWorkflow)); @@ -432,40 +428,42 @@ mod tests { } #[test] - fn merge_skill_priority_workspace_over_user() { + fn merge_skill_first_fed_wins_on_collision() { 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 { + let first_path = std::path::PathBuf::from("/a/skills/x/SKILL.md"); + let second_path = std::path::PathBuf::from("/b/skills/x/SKILL.md"); + let first = WorkflowRecord { slug: Slug::parse("x").unwrap(), - description: "ws".into(), + description: "first".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"), + body: "first body".into(), + path: first_path.clone(), + source: WorkflowSource::Skill { + dir: std::path::PathBuf::from("/a/skills"), }, }; - let user_record = WorkflowRecord { + let second = WorkflowRecord { slug: Slug::parse("x").unwrap(), - description: "user".into(), + description: "second".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"), + body: "second body".into(), + path: second_path.clone(), + source: WorkflowSource::Skill { + dir: std::path::PathBuf::from("/b/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 { .. })); + // Caller is responsible for feeding in priority order; the + // registry just keeps whichever arrives first. + assert!(reg.merge_skill(first).is_none()); + let shadow = reg + .merge_skill(second) + .expect("later-fed skill must shadow"); + assert_eq!(shadow.kept_path, first_path); + assert!(matches!(shadow.kept_source, WorkflowSource::Skill { .. })); } #[test] @@ -474,15 +472,15 @@ mod tests { 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_source: WorkflowSource::Skill { + dir: std::path::PathBuf::from("/skills"), }, - shadowed_path: std::path::PathBuf::from("/user/skills/x/SKILL.md"), + shadowed_path: std::path::PathBuf::from("/skills/x/SKILL.md"), }; let msg = s.message(); assert!(msg.contains("/x")); assert!(msg.contains("workspace workflow")); - assert!(msg.contains("user skill")); + assert!(msg.contains("skill")); } #[test] diff --git a/crates/pod/src/pod.rs b/crates/pod/src/pod.rs index 36c1a475..c014a73f 100644 --- a/crates/pod/src/pod.rs +++ b/crates/pod/src/pod.rs @@ -2824,36 +2824,23 @@ fn prepare_pod_common_from_scope( /// 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. +/// Skills come exclusively from the manifest's `[skills] directories` +/// list (resolved against the manifest base directory). Internal +/// Workflows already loaded via [`memory::load_workflows`] take priority +/// over skills sharing the same slug; collisions are surfaced as +/// [`memory::ShadowedSkill`] events that the caller pushes onto the +/// Pod's 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 Some(skills_cfg) = manifest.skills.as_ref() else { + return shadows; + }; + for dir in &skills_cfg.directories { + for skill in memory::load_skills_from_dir(dir) { + let source = memory::WorkflowSource::Skill { dir: dir.clone() }; let record = skill.into_workflow_record(source); if let Some(shadow) = registry.merge_skill(record) { shadows.push(shadow); @@ -2895,29 +2882,22 @@ fn build_scope_with_memory(manifest: &PodManifest, pwd: &Path) -> Result 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, + let Some(skills_cfg) = manifest.skills.as_ref() else { + return Vec::new(); + }; + skills_cfg + .directories + .iter() + .map(|dir| ScopeRule { + target: dir.clone(), permission: Permission::Read, recursive: true, - }); - } - rules + }) + .collect() } /// Snapshot the process's current working directory as the Pod's pwd, @@ -3054,15 +3034,19 @@ permission = "write" } #[test] - fn skill_dir_read_rules_ignores_missing_skills_section() { + fn skill_dir_read_rules_empty_when_skills_section_missing() { 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); - } + assert!(rules.is_empty()); + } + + #[test] + fn ingest_skills_returns_empty_when_skills_section_missing() { + let manifest = minimal_manifest_with_skills(vec![]); + let mut registry = memory::WorkflowRegistry::empty(); + let shadows = ingest_skills(&mut registry, &manifest); + assert!(shadows.is_empty()); + assert!(registry.is_empty()); } #[test] diff --git a/tickets/agent-skills.md b/tickets/agent-skills.md index 67f86b22..9e4b64e3 100644 --- a/tickets/agent-skills.md +++ b/tickets/agent-skills.md @@ -37,17 +37,19 @@ SKILL.md frontmatter: ### ロードソース -- **`$user/skills//SKILL.md`** — `$XDG_CONFIG_HOME/insomnia/skills/` 配下。**デフォルトで有効** -- **`$workspace/skills/`** — **デフォルトで無効**。manifest の `[skills]` セクションで明示指定したパスのみ ingest する +skill は **manifest の `[skills] directories` で明示指定したパスのみ** ingest する。`$config_dir/skills/` の自動ロードや builtin skills は持たない — manifest に書かれていないディレクトリは決して読まれない。 - ```toml - [skills] - directories = [".claude/skills", ".cursor/skills"] - ``` +```toml +[skills] +directories = ["~/skills", ".claude/skills", ".cursor/skills"] +``` - 各パスは workspace root からの相対 or 絶対。manifest の base directory に対して resolve する（既存 path 解決と同方針）。Claude Code / Cursor 等が既に書いている `.claude/skills/` `.cursor/skills/` をそのまま流用できることが目的。 +各パスは manifest の base directory に対して resolve する（既存 `scope.allow.target` 等と同方針）。絶対パスでも相対パスでもよく: -- ビルトイン `$insomnia/skills/` は不要になるまで作らない（前ガイドラインのまま） +- user manifest layer (`$config_dir/manifest.toml`) に書けば全 workspace 共通で使う skill 集を持てる +- project manifest layer に書けば workspace 固有の `.claude/skills/` `.cursor/skills/` をそのまま流用できる + +ビルトイン `$insomnia/skills/` は不要になるまで作らない（前ガイドラインのまま）。 ### SKILL → Workflow マッピング @@ -72,8 +74,7 @@ skill ディレクトリ全体（`SKILL.md` 本体だけでなく `scripts/` `re 同一 slug が複数ソースから来た場合の優先順位: 1. `/.insomnia/memory/workflow/.md`（内製 Workflow） -2. workspace skills（manifest 指定パス） -3. user skills（`$user/skills/`） +2. manifest `[skills] directories` の各エントリ（**列挙順**で解決。先に書かれたディレクトリが上位） 衝突時は上位を採用し、shadow した側について `Event::Notification` を発行する。「明示的に書かれた内製 Workflow が外部資産より強い」順に並べる。 @@ -93,20 +94,19 @@ skill ディレクトリ全体（`SKILL.md` 本体だけでなく `scripts/` `re ## 完了条件 -- `$user/skills/` 配下の SKILL.md が Workflow として登録され、`/` で呼び出せる -- manifest で `[skills] directories = [...]` を指定した workspace では、そのパス配下の SKILL.md だけが追加で ingest される。指定しない workspace では workspace 側 skill は 0 件 +- manifest の `[skills] directories = [...]` で指定したパス配下の SKILL.md が Workflow として登録され、`/` で呼び出せる +- `[skills]` セクションが無い manifest では skill 由来の Workflow は 0 件 (`$config_dir/skills/` 等の暗黙ロードはしない) - 内製 Workflow と同 slug の skill は内製優先で shadow され、Notification が発行される - skill ディレクトリ（SKILL.md 本体・`scripts/`・`references/`・`assets/`）が scope readable に含まれ、agent が Read ツールでアクセスできる - frontmatter 違反の skill は warn でスキップされ、他の skill / Pod 起動は影響を受けない -- 単体テストで frontmatter 検証、Workflow へのマッピング、衝突解決（内製 > workspace > user）、manifest 未指定時の workspace skip が verify される +- 単体テストで frontmatter 検証、Workflow へのマッピング、衝突解決（内製 > skill）、manifest 未指定時の skill 0 件が verify される ## 実装順序 1. SKILL.md パーサと frontmatter 検証を実装。Workflow frontmatter への変換器を含めてテスト完結 -2. `$user/skills/` の loader を Workflow registry に接続 -3. manifest に `[skills] directories: Vec` を追加し、workspace 側 ingest を実装 -4. 衝突解決と Notification 発行を乗せる -5. skill ディレクトリの Scope union（read 自動 allow） +2. manifest に `[skills] directories: Vec` を追加し、ingest 経路を実装 +3. 衝突解決と Notification 発行を乗せる +4. skill ディレクトリの Scope union（read 自動 allow） 各ステップ終了時点でビルド通過・既存テスト合格を維持する。 @@ -121,6 +121,9 @@ skill ディレクトリ全体（`SKILL.md` 本体だけでなく `scripts/` `re ## Review -- 状態: Approve +- 状態: Approve (Round 2) - レビュー詳細: [./agent-skills.review.md](./agent-skills.review.md) -- 日付: 2026-05-04 +- 経緯: + - 2026-05-04: 初回レビュー Approve (旧仕様: 2 ソース構成) + - 2026-05-05: `$config_dir/skills/` の自動 ingest を廃止、skill ロードソースを manifest `[skills] directories` のみに統合する設計変更。実装も `WorkflowSource::WorkspaceSkill` / `UserSkill` 2 バリアントから単一の `Skill { dir }` に簡略化済み + - 2026-05-04: Round 2 レビュー Approve。新設計の完了条件をすべて満たし、Round 1 で挙げた user_skills_dir 関連の懸念は設計変更で構造的に解消。non-blocking として manifest crate の stale doc コメント (`crates/manifest/src/lib.rs:47-62`) と受け皿チケット (`tickets/permission-extension-point.md:61`) の旧バリアント名参照が残るのみ diff --git a/tickets/agent-skills.review.md b/tickets/agent-skills.review.md index 6a998eb7..b7784d07 100644 --- a/tickets/agent-skills.review.md +++ b/tickets/agent-skills.review.md @@ -1,47 +1,85 @@ # Review: Agent Skills を Workflow として ingest -## 前提・要件の確認 +## Round 1 (2026-05-04, 旧仕様) -### 完了条件マッピング +初回レビュー時点では「manifest workspace skills + `$config_dir/skills/` 自動 ingest」の 2 ソース構成を前提に **Approve** とした。`WorkflowSource` も `WorkspaceSkill` / `UserSkill` の 2 バリアントだった。Round 1 の本文は git 履歴で参照可能 (差し戻し履歴の保全)。 -| 条件 | 状態 | 根拠 | +その後の設計変更で「skill ロードソースは manifest `[skills] directories` のみ。`$config_dir/skills/` の暗黙ロードと `paths::user_skills_dir()` は廃止」へ移行し、`WorkflowSource` も単一バリアント `Skill { dir }` に統合された。本ファイルは新設計に対する Round 2 のレビューに書き換える。 + +--- + +## Round 2 (2026-05-04, 設計変更後) + +### 前提・要件の確認 (新仕様の完了条件マッピング) + +| 完了条件 | 状態 | 根拠 | |---|---|---| -| `$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) | 後述「テストカバレッジ」参照 | +| manifest `[skills] directories` で指定したパス配下の SKILL.md が Workflow として登録され `/` で呼べる | 満 | `crates/pod/src/pod.rs:2833-2851` の `ingest_skills` が `manifest.skills.directories` を順に走査し `merge_skill`。生成 `WorkflowRecord` は `model_invokation: true`/`user_invocable: true`/`requires: []` 固定 (`crates/memory/src/skill.rs:73-84`) なので既存 `/` resolver と `list_user_invocable` がそのまま拾う | +| `[skills]` 未指定時は skill 由来 Workflow 0 件 | 満 | `ingest_skills` (`crates/pod/src/pod.rs:2837-2840`) は `manifest.skills.as_ref()` が `None` なら即 return。`paths::user_skills_dir()` も `crates/manifest/src/paths.rs` から削除済みで暗黙ロード経路自体が存在しない。テスト `ingest_skills_returns_empty_when_skills_section_missing` (`crates/pod/src/pod.rs:3043-3050`) で覆われている | +| 同一 manifest 内の `directories` 列挙順 = 優先順位 (先に書かれたものが上位) | 満 | `merge_skill` は first-insert wins (`crates/memory/src/workflow.rs:142-154`)、`ingest_skills` は `for dir in &skills_cfg.directories` で vec の先頭から feed する。テスト `merge_skill_first_fed_wins_on_collision` (`crates/memory/src/workflow.rs:431-467`) で契約を verify | +| 内製 Workflow 優先で shadow + Notification 発行 | 満 | `merge_skill` は既存 record を保持して `ShadowedSkill` を返す。`drain_skill_shadows` (`crates/pod/src/pod.rs:2855-2862`) が `push_notify` で `[Skill shadowed] ...` を Pod の notify バッファへ。`merge_skill_shadows_existing_workflow` (`crates/memory/src/workflow.rs:401-428`) で workspace workflow > skill の優先関係を verify | +| skill ディレクトリ全体 (`SKILL.md` + `scripts/` + `references/` + `assets/`) が scope readable | 満 | `skill_dir_read_rules` (`crates/pod/src/pod.rs:2888-2901`) が manifest `[skills] directories` の各 entry を `Permission::Read`+`recursive: true` で `scope_config.allow.extend`。manifest 未指定時は空 Vec を返す (`crates/pod/src/pod.rs:2889-2891`) ので「設定無しでも `~/.config/insomnia/skills` を allow に乗せる」Round 1 の懸念は構造的に消滅している | +| frontmatter 違反は warn でスキップ、他 skill / Pod 起動は止まらない | 満 | `load_skills_from_dir` (`crates/memory/src/skill.rs:202-248`) は per-entry `match` で `tracing::warn!`。`Pod::from_manifest` 経路は `Result` を返さない | +| 単体テスト (frontmatter / mapping / 衝突解決 / manifest 未指定) | 満 | 後述「テストカバレッジ」参照 | -### 方針マッピング +### アーキテクチャ・スコープ -- ロードソースの優先順 (内製 → 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 cascade**: `SkillsConfig::merge` (`crates/manifest/src/config.rs:218-222`) は `self.directories.extend(upper.directories)` で他の Vec 系設定 (scope rules) と同じ累積規則。`resolve_paths` (`crates/manifest/src/config.rs:189-193`) は他 path フィールドと同じ `join_if_relative(base, dir)`。`TryFrom` で `ensure_absolute("skills.directories", ...)` を呼ぶ三段構え (`crates/manifest/src/config.rs:430-434`) も既存パターンに沿う +- **scope union**: `build_scope_with_memory` (`crates/pod/src/pod.rs:2874-2882`) が `deny_write_rules` と `skill_dir_read_rules` を `extend` する形は `scope::deny_write_rules` の参照モデルと完全に対称 +- **WorkflowRegistry**: `merge_skill` の「first-fed wins、再ランクしない」契約は doc コメントで明示されており、`ingest_skills` 側が priority 順 feed の責任を負う。registry の不変条件を呼び出し側が守る、よくある分離 +- **push_notify**: `[Skill shadowed]` prefix は既存の `[ ...]` 文体と整合。`Method::Notify` の既存ルートを再利用しており、新たな event 種別の追加は無い +- **Cargo deps**: `serde_yaml` / `tempfile` / `tracing` / `thiserror` はすべて memory crate に既存、`cargo add` 縛りに抵触する追加なし -## アーキテクチャ・スコープ +#### 範囲外項目への侵入チェック +- `allowed-tools` 実効化 — 実装無し。`SkillFrontmatter::allowed_tools` は受信し warn して捨てる (`crates/memory/src/skill.rs:156-161`)、受け皿契約は `tickets/permission-extension-point.md` に記載 +- `scripts/` 自動実行 — 無し。Read 経由の閲覧のみ +- ビルトイン skill (`$insomnia/skills/`) — 実装無し +- skill 自動生成 / skill 間依存 — 実装無し +- 新規 protocol message / event 種別 — 無し -### 既存パターンとの整合 +新仕様の Scope に過剰実装は無い。 -- **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 縛り) には抵触しない +#### 設計変更の波及確認 (`WorkflowSource` 単一バリアント化) -### 範囲外項目への侵入チェック +旧 2 バリアント (`WorkspaceSkill` / `UserSkill`) を `Skill { dir }` に統合した変更の影響範囲を確認: -- `allowed-tools` 実効化: 行わず warn 止まり。`tickets/permission-extension-point.md` 側に受け皿契約を追記しているのは妥当な情報の遺し方で、本チケットのスコープ外実装には踏み込んでいない -- `scripts/` 自動実行: しない。Read 経由の閲覧のみで、Bash 経由実行は Permission 層待ち -- ビルトイン skill / 自動生成 / 間依存: いずれも実装無し -- skill 用 protocol message / event 種別の追加: 無し。`Method::Notify` の既存ルートを再利用 +- `WorkflowSource::label()` (`crates/memory/src/workflow.rs:38-46`) は `ShadowedSkill::message()` でしか使われない。旧仕様では「shadow されたのは workspace skill か user skill か」を一行で区別したかったが、新仕様では skill ソースは 1 種類で十分。`label()` の戻り値も `"workspace workflow"` / `"skill"` の 2 値で、メッセージ文字列内で `dir` を `display()` するため出所も特定可能。**user/workspace 区別が必要だった他箇所は無い** +- `permission-extension-point.md` (受け皿チケット) で「`WorkflowRecord` の出所は `WorkflowSource::Skill { dir }` で識別済み」とすべき箇所 — 受け皿実装時の足がかり情報として旧 2 バリアント名で書かれている +- `crates/manifest/src/lib.rs` の `SkillsConfig` および `pub skills: Option` の doc コメント — 旧仕様の「user-level `$config_dir/skills/` is always probed regardless of this field」「in addition to the user-level `$config_dir/skills/`」が残存 -### 不必要な抽象 / 過剰な API のチェック +#### 不必要な抽象 / 過剰な API のチェック +- `SkillRecord` を一旦経由してから `into_workflow_record` で `WorkflowRecord` に投影する 2 段構造は当面 SkillRecord 独自情報を持たないが、Permission 層が来たときに `allowed_tools` 等 skill 固有 metadata を保持する受け皿として機能する。許容範囲 +- `WorkflowSource::label()` は `ShadowedSkill::message()` の 1 caller のみ。`message()` 内に inline しても良いが、`label()` を独立メソッドとして晒しても害は無く、将来 UI / log で再利用される可能性は残せる。許容範囲 -- `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 側で抽象化を増やすより素直 +### テストカバレッジ + +ticket の完了条件にある「frontmatter 検証 / Workflow マッピング / 衝突解決 / manifest 未指定時 0 件」を順に確認: + +- 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` を確認 +- 衝突解決: + - 内製 Workflow > skill: `merge_skill_shadows_existing_workflow` (`crates/memory/src/workflow.rs:401-428`) + - skill ↔ skill (列挙順 = 優先順): `merge_skill_first_fed_wins_on_collision` (`crates/memory/src/workflow.rs:431-467`) + - 衝突メッセージ: `shadow_message_is_human_readable` (`crates/memory/src/workflow.rs:470-484`) +- manifest 未指定時: + - `ingest_skills_returns_empty_when_skills_section_missing` (`crates/pod/src/pod.rs:3043-3050`) + - `skill_dir_read_rules_empty_when_skills_section_missing` (`crates/pod/src/pod.rs:3036-3041`) +- 通常経路: `ingest_skills_loads_from_workspace_directories` (`crates/pod/src/pod.rs:3052-3071`) + +設計変更で「user vs workspace の優先順位」を verify するテストは不要となり (Round 1 の `merge_skill_priority_workspace_over_user` 相当が削除)、代わりに「first-fed wins 契約」を verify するテストに置き換えられている。新契約に整合的。 + +`cargo test --workspace --no-fail-fast` 全 crate で 0 failed を確認。 + +### Round 1 の指摘事項の引継ぎ + +- ✅ **解消**: 「scope.allow が常に user_skills_dir を含む / opt-out 手段が無い」 — `paths::user_skills_dir()` が削除され、manifest 未指定時に scope への union も発生しない (`skill_dir_read_rules` の早期 return)。`[skills]` を書かない = 完全 opt-out +- ✅ **解消**: 「Round 1 nit: `pod.rs` の `Permission` import 整形」 — 新コード (`crates/pod/src/pod.rs:2880, 2895-2898`) では `Permission::Read` を直接参照しており、import の整形問題は発生していない +- 引継ぎ (Round 2 でも有効): + - **`merge_skill` の事前条件依存性**: 「降順 priority feed を呼び出し側が守る」契約は doc + テストで明示されているが debug assertion 無し。`ingest_skills` 以外の caller が増えたときに気付きにくい。follow-up で防衛コードを足す余地はある (non-blocking) + - **`Slug::parse` が agent-skills 仕様より厳しい**: 仕様は `[a-z0-9-]+` だが内製 `Slug` は先頭末尾 `-` と `--` を禁止。実用的問題は無いが docs/plan で明示する価値あり + - **`drain_skill_shadows` を resume 時にも emit する仕様の暗黙性**: `crates/pod/src/pod.rs:2501` で resume 時にも shadow Notification を再注入する設計。registry を再構築する以上一貫しているが、ユーザー観点では一行コメントを残しておくと将来の再評価が楽 + - **shadow 経路の end-to-end テスト**: `merge_skill` の戻り値検証はあるが `Pod::from_manifest` → `push_notify` 到達の統合テストは無い (致命的でない) ## 指摘事項 @@ -51,30 +89,19 @@ ### 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 に回せる +- **stale doc コメント (manifest crate)** — `crates/manifest/src/lib.rs:47-62` が旧仕様前提のまま: + - L47-48: 「in addition to the user-level `$config_dir/skills/`」 + - L57-62: `SkillsConfig` の doc が「Off by default at the workspace level; user-level `$config_dir/skills/` is always probed regardless of this field.」と明確に誤った仕様を述べている + + 動作には影響しないが、将来 `skills` を触る開発者が誤読する。書き換え推奨 (例: 「`directories` で明示指定したパスのみ ingest される。デフォルト ingest は無い」) +- **stale 参照 (cross-ticket)** — `tickets/permission-extension-point.md:61` が旧 2 バリアント名 (`WorkflowSource::WorkspaceSkill { dir }` / `UserSkill { dir }`) で受け皿実装の足がかりを書いている。本チケットの責務外だが、本チケットの設計変更に伴って整合性が崩れた点なので follow-up で受け皿チケット側を更新するのが筋 +- **Round 1 から引き継ぐ非 blocking 事項** — `merge_skill` 防衛 assert / `Slug` 厳格化の docs 周知 / resume 時 shadow 再注入のコメント / shadow E2E テスト (本セクション「Round 1 の指摘事項の引継ぎ」末尾参照) ### 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 を確認。 +- `crates/memory/src/skill.rs:316-326` (`invalid_slug_name_is_error`): `BAD-Caps` 入力の `InvalidName` / `NameDirMismatch` どちらが先に発火するかをテストで明示しておくと validation 順序を変えたときの regression を捕まえやすい (Round 1 から継続) +- `crates/pod/src/pod.rs:3016` のテスト名 `skill_dir_read_rules_lists_workspace_skill_directories` は新仕様だと `workspace_skill` という呼称が違和感ある (skill ソースに workspace/user 区別はもう無い)。`..._lists_manifest_directories` 等への rename を検討 (cosmetic) ## 判断 -**Approve** — チケットの完了条件は要件・テストカバレッジともすべて満たされており、既存の manifest cascade / scope union / push_notify / WorkflowRegistry の各パターンに沿って実装されている。範囲外項目 (`allowed-tools` 実効化、`scripts/` 自動実行、ビルトイン skill 等) には踏み込まず、受け皿契約の追記も適切。Non-blocking の follow-up は将来チケットで巻き取って良い性質のもの。 +**Approve** — 設計変更後の完了条件 6 点すべてが対応する実装・テストで verify されており、scope 外項目に踏み込んでいない。`WorkflowSource` 単一バリアント化の波及は本実装内で閉じており、user/workspace 区別が本質的に必要だった箇所は無い。Round 1 で挙げた「scope に常時 user_skills_dir が乗る」「opt-out 手段が無い」という最大の懸念は設計変更で構造的に解消した。Non-blocking として残るのは stale doc コメント (manifest crate / 受け皿チケット) と Round 1 から引き継いだ補強候補で、いずれも別 commit / 別チケットで巻き取って良い性質。 diff --git a/tickets/permission-extension-point.md b/tickets/permission-extension-point.md index ab401fc1..8554324a 100644 --- a/tickets/permission-extension-point.md +++ b/tickets/permission-extension-point.md @@ -58,7 +58,7 @@ action = "allow" 本チケットの 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`) +- `WorkflowRecord` の出所は `WorkflowSource::Skill { dir }` で識別済み (`crates/memory/src/workflow.rs`)。`dir` は manifest `[skills] directories` に書かれた skill ルートそのもの - 受け皿実装時に `SkillFrontmatter::allowed_tools` の保持先を `WorkflowRecord` に伸ばすか、別の SkillRecord registry を持つかは本チケット内で決める - 現状の `tracing::warn!` は受け皿実装と同時に消す