update: Agent skills実装のレビュー・対応
This commit is contained in:
parent
dec17c9909
commit
9d709c6470
|
|
@ -45,21 +45,21 @@ pub struct PodManifest {
|
|||
#[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.
|
||||
/// Workflows. 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. Absent ⇒ no skills loaded;
|
||||
/// there is no implicit `$config_dir/skills/` or builtin probe.
|
||||
#[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.
|
||||
/// 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
|
||||
|
|
|
|||
|
|
@ -79,12 +79,6 @@ 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"))
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
/// `<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
|
||||
/// 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 },
|
||||
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<ShadowedSkill> {
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -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<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 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<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.
|
||||
/// will ingest from the manifest's `[skills] directories`. 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,
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -37,17 +37,19 @@ SKILL.md frontmatter:
|
|||
|
||||
### ロードソース
|
||||
|
||||
- **`$user/skills/<name>/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. `<workspace>/.insomnia/memory/workflow/<slug>.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 として登録され、`/<name>` で呼び出せる
|
||||
- manifest で `[skills] directories = [...]` を指定した workspace では、そのパス配下の SKILL.md だけが追加で ingest される。指定しない workspace では workspace 側 skill は 0 件
|
||||
- manifest の `[skills] directories = [...]` で指定したパス配下の SKILL.md が Workflow として登録され、`/<name>` で呼び出せる
|
||||
- `[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<PathBuf>` を追加し、workspace 側 ingest を実装
|
||||
4. 衝突解決と Notification 発行を乗せる
|
||||
5. skill ディレクトリの Scope union（read 自動 allow）
|
||||
2. manifest に `[skills] directories: Vec<PathBuf>` を追加し、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`) の旧バリアント名参照が残るのみ
|
||||
|
|
|
|||
|
|
@ -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 として登録され `/<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) | 後述「テストカバレッジ」参照 |
|
||||
| manifest `[skills] directories` で指定したパス配下の SKILL.md が Workflow として登録され `/<name>` で呼べる | 満 | `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`) なので既存 `/<slug>` 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<PodManifestConfig>` で `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 は既存の `[<Domain> ...]` 文体と整合。`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<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 縛り) には抵触しない
|
||||
#### 設計変更の波及確認 (`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<SkillsConfig>` の 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 / 別チケットで巻き取って良い性質。
|
||||
|
|
|
|||
|
|
@ -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!` は受け皿実装と同時に消す
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user