yoi/tickets/agent-skills.review.md

# Review: Agent Skills を Workflow として ingest

## 前提・要件の確認

### 完了条件マッピング

| 条件 | 状態 | 根拠 |
|---|---|---|
| `$user/skills/` 配下の `SKILL.md` が Workflow として登録され `/<name>` で呼び出せる | 満 | `crates/pod/src/pod.rs:2849-2862` で `manifest::paths::user_skills_dir()` を `load_skills_from_dir` → `merge_skill`。生成 `WorkflowRecord` は `user_invocable: true`/`model_invokation: true` (`crates/memory/src/skill.rs:73-84`) なので既存 resolver と `list_user_invocable` がそのまま拾う |
| manifest `[skills] directories` 指定 workspace で追加 ingest、未指定なら 0 件 | 満 | `crates/pod/src/pod.rs:2840-2847` が `manifest.skills.as_ref()` 配下のみ走査。未指定なら `if let Some(...)` を抜けるだけで何も読まない |
| 内製 Workflow と同 slug の skill は内製優先で shadow し Notification を発行 | 満 | `WorkflowRegistry::merge_skill` (`crates/memory/src/workflow.rs:146-158`) が既存 record 保持、`ShadowedSkill` を返す。`drain_skill_shadows` (`crates/pod/src/pod.rs:2868-2876`) で `push_notify` |
| skill ディレクトリ全体 (`SKILL.md`/`scripts/`/`references/`/`assets/`) が scope readable | 満 | `skill_dir_read_rules` (`crates/pod/src/pod.rs:2902-2922`) が `Permission::Read` + `recursive: true` で `build_scope_with_memory` の `scope_config.allow.extend` に乗る |
| frontmatter 違反は warn でスキップ、他 skill / Pod 起動に影響しない | 満 | `load_skills_from_dir` (`crates/memory/src/skill.rs:202-248`) が `parse_skill_md` のエラーを `tracing::warn!` で握り潰し、結果を Vec に積む。Pod 構築フローは `Result` を返さない |
| 単体テスト: frontmatter 検証 / Workflow マッピング / 衝突解決 / manifest 未指定時 workspace skip | 概ね満 (1 点 nit) | 後述「テストカバレッジ」参照 |

### 方針マッピング

- ロードソースの優先順 (内製 → workspace → user) — `ingest_skills` (`crates/pod/src/pod.rs:2836-2867`) で実現。registry にあらかじめ載った内製 Workflow が `merge_skill` で常に勝ち、workspace を user より先に feed することで spec 通りの順位が出る
- `model_invokation: true` / `user_invocable: true` 固定、`requires` 空 — `SkillRecord::into_workflow_record` (`crates/memory/src/skill.rs:73-84`) で固定
- `allowed-tools` は warn で受け流し — `crates/memory/src/skill.rs:156-161` で `tracing::warn!`。Permission 層の受け皿契約は `tickets/permission-extension-point.md` 側に追記済み
- 検証は内製 Workflow より緩く個別 skill 単位 skip — `load_skills_from_dir` の per-entry `match` で `tracing::warn!` のみ。Pod 構築は止まらない

## アーキテクチャ・スコープ

### 既存パターンとの整合

- **manifest/scope 拡張**: `SkillsConfig` の `merge` は他の `merge_option` と同じ pattern (`crates/manifest/src/config.rs:215-220`)。`resolve_paths` は既存の `auth.path` などと同様 `join_if_relative(base, dir)` を使い、`TryFrom<PodManifestConfig>` で `ensure_absolute` を呼ぶ三段構え。ScopeConfig の他 field と同じ流儀で破綻なし
- **memory crate**: `skill.rs` を新ファイルに切り出し、`workflow.rs` には `WorkflowSource` / `ShadowedSkill` / `merge_skill` のみを足す。`WorkflowRecord` 自身に skill 起源固有の field を生やしていないので、`load_workflows` 側のエラー型 (`WorkflowLoadError`) はそのまま。SKILL 用エラー型は `SkillParseError` に分離 — 内製 Workflow と外部 skill で「壊れたら hard error / 壊れたら warn skip」の温度差が型レベルで表現されている
- **scope union pattern**: `build_scope_with_memory` が `deny_write_rules`/`skill_dir_read_rules` を `extend` する構造。`scope::deny_write_rules` の参照モデルと完全に対称
- **`paths::user_skills_dir`**: `user_prompts_dir` (`crates/manifest/src/paths.rs:78-81`) と並列に並ぶ命名・実装。違和感なし
- **Cargo**: `serde_yaml` / `tempfile` / `tracing` / `thiserror` は既に memory の依存にあり (`crates/memory/Cargo.toml:16-22`)。新規追加は無く、Cargo 系のガイドライン (cargo add 縛り) には抵触しない

### 範囲外項目への侵入チェック

- `allowed-tools` 実効化: 行わず warn 止まり。`tickets/permission-extension-point.md` 側に受け皿契約を追記しているのは妥当な情報の遺し方で、本チケットのスコープ外実装には踏み込んでいない
- `scripts/` 自動実行: しない。Read 経由の閲覧のみで、Bash 経由実行は Permission 層待ち
- ビルトイン skill / 自動生成 / 間依存: いずれも実装無し
- skill 用 protocol message / event 種別の追加: 無し。`Method::Notify` の既存ルートを再利用

### 不必要な抽象 / 過剰な API のチェック

- `WorkflowSource::label()` は `ShadowedSkill::message()` でしか使われない (`crates/memory/src/workflow.rs:42-49`)。public で晒されているが UI/log の他の経路で再利用される含みはあるので許容範囲
- `SkillRecord` を一旦経由してから `into_workflow_record` で `WorkflowRecord` に投影する 2 段構造は、当面 `SkillRecord` の独自情報 (allowed_tools 等) を持たせる場が無いので冗長気味。ただし「Permission 層が来たときに skill 固有 metadata の保持先になる」という拡張ポイントとして見れば、無駄ではない (受け皿チケット側の文面とも整合)
- `WorkflowRegistry::merge_skill` の「呼び出し側に降順 priority feed を強制し registry は再ランクしない」契約は doc comment で明記されており、ingest 側 `ingest_skills` でその順序を遵守。registry 側で抽象化を増やすより素直

## 指摘事項

### Blocking

なし。

### Non-blocking / Follow-up

- **`restore_from_manifest` でも shadow 通知を emit する設計の説明不足** — `crates/pod/src/pod.rs:2501` で resume 時にも `drain_skill_shadows` を呼ぶ。実装ロジックとしては「registry を再構築する以上、衝突は再評価される」という意味で一貫しているが、ユーザーから見ると同じ session が一度落ちて起動するたびに `[Skill shadowed]` が再注入される。`drain_skill_shadows` か `restore_from_manifest` 周辺に「resume 時にも (重複で) 出すのは仕様」と一行コメントを残しておくと、後で疑問が再燃しない
- **`WorkflowRegistry::merge_skill` の事前条件依存性** — 「降順 priority feed を呼び出し側が守る」契約は doc comment にあるが、debug assertion (例えば 2 回目以降の skill record が既存 skill record を shadow しようとしたとき、`ws/user` の順序が逆だったら `debug_assert!`) は無い。将来 `ingest_skills` 以外の caller が増えたときに気付きにくいので、follow-up で防衛コードを足す余地あり
- **`Slug::parse` が agent-skills 仕様より厳しい** — 仕様は `[a-z0-9-]+` だが `Slug` は先頭末尾の `-` と `--` を禁止。今 `[a-z0-9-]+` 厳密準拠の skill は実用上ほぼ無く、内製 Workflow の slug ルールと一致させる方が合理的なので、現状の厳しめ運用で良い。ただしユーザー観点での挙動 (warn で skip される) を docs/plan で明示しておく価値はある
- **scope.allow が常に user_skills_dir を含む** — `skill_dir_read_rules` (`crates/pod/src/pod.rs:2902-2922`) は manifest 側に skills 設定が無くとも `~/.config/insomnia/skills` を allow に追加する。`resolve_path` は存在しない path を最寄りの祖先に anchor して許容するので技術的には壊れないが、「ユーザーがそのディレクトリを意図せず作っただけで Pod の scope に readable として乗る」という挙動は ticket の背景 (デフォルトで有効) に沿っており妥当。ただし「ユーザーが skills 機能を完全に opt-out したい」場合の手段が現状無いので、Phase 2 で `[skills] enabled = false` 等の opt-out を検討する余地あり
- **`pod.rs` のテスト 3 件が `skill_dir_read_rules`/`ingest_skills` のホワイトボックステスト** — 衝突発生時の `shadow` Notify 経路 (`drain_skill_shadows` が `push_notify` を確実に呼ぶこと) を end-to-end で検証するテストはまだ無い。`memory/src/workflow.rs` 側で `merge_skill` の戻り値検証はあるので致命的ではないが、Pod 構築から Notification 到達までの統合的な確認は将来 follow-up に回せる

### Nits

- `crates/memory/src/skill.rs:316-326` (`invalid_slug_name_is_error`): `BAD-Caps` という入力は dir 名としても禁止されている (大文字を含むため `Slug::parse` が rejected する)。コメントで「`InvalidName` か `NameDirMismatch` のどちらかになる」と書いてあるが、実際にどちらが先に発火するかをテストで明示しておくと、validation 順序を変えたときに気付きやすい
- `crates/pod/src/pod.rs:14-16`: `Permission` を import 行に追加した整形で行が 100 文字を超えそうに見える (rustfmt は通っているはずなので問題ないが、`use manifest::{}` ブロックがリフォーマットしたほうが読みやすい)
- `[Skill shadowed]` 文言は agent 側に渡る system message としては短くて十分だが、今後類似のシステム由来 notify が増えたときに prefix の語彙統一 (`[Memory shadowed]` / `[Knowledge ...]` 等) を docs/plan で揃える価値あり

## テストカバレッジ

ticket の完了条件にある「frontmatter 検証、Workflow へのマッピング、衝突解決、manifest 未指定時の workspace skip」を順に確認:

- frontmatter 検証: `parses_minimal_skill` / `name_dir_mismatch_is_error` / `invalid_slug_name_is_error` / `empty_description_is_error` / `description_at_cap_is_accepted` / `description_over_cap_is_error` / `missing_frontmatter_is_error` / `extra_frontmatter_fields_are_kept` (`crates/memory/src/skill.rs`) — 過不足なし
- Workflow マッピング: `into_workflow_record_uses_skill_defaults` (`crates/memory/src/skill.rs:415-435`) で `model_invokation`/`user_invocable`/`requires`/`source` を確認
- 衝突解決:
  - `merge_skill_inserts_when_no_collision` / `merge_skill_shadows_existing_workflow` / `merge_skill_priority_workspace_over_user` / `shadow_message_is_human_readable` (`crates/memory/src/workflow.rs:396-490`)
- manifest 未指定時の workspace skip: 直接の単体テストは無いが、`ingest_skills_loads_from_workspace_directories` (`crates/pod/src/pod.rs:3068-3087`) で `manifest.skills = Some(...)` のケースを cover、`skill_dir_read_rules_ignores_missing_skills_section` (`crates/pod/src/pod.rs:3057-3066`) が Skill 設定無しでの allow rules を検査。`ingest_skills` の構造的に未指定なら for ループに入らない事は自明だが、念のため「未指定 manifest で `ingest_skills` が空 Vec を返す」テストを 1 件足すとカバレッジが完結する (non-blocking)

`cargo test --workspace --no-fail-fast` を実行: 全 crate で 0 failed を確認。

## 判断

**Approve** — チケットの完了条件は要件・テストカバレッジともすべて満たされており、既存の manifest cascade / scope union / push_notify / WorkflowRegistry の各パターンに沿って実装されている。範囲外項目 (`allowed-tools` 実効化、`scripts/` 自動実行、ビルトイン skill 等) には踏み込まず、受け皿契約の追記も適切。Non-blocking の follow-up は将来チケットで巻き取って良い性質のもの。