pod-factory完了
This commit is contained in:
parent
e8a5fe557a
commit
2052ac498c
1
TODO.md
1
TODO.md
|
|
@ -6,7 +6,6 @@
|
|||
- [ ] Protocol の設計 → [tickets/protocol-design.md](tickets/protocol-design.md)
|
||||
- [ ] パーミッション: パターンベースのツール実行制御 → [tickets/permission-extension-point.md](tickets/permission-extension-point.md)
|
||||
- [ ] ネイティブ GUI クライアント MVP → [tickets/native-gui-mvp.md](tickets/native-gui-mvp.md)
|
||||
- [ ] Pod Factory: カスケード設定とプロンプト資産 → [tickets/pod-factory.md](tickets/pod-factory.md)
|
||||
- [ ] TUI 拡充
|
||||
- [ ] Pod の明示的 shutdown → [tickets/tui-pod-shutdown.md](tickets/tui-pod-shutdown.md)
|
||||
- [ ] 新しい Pod を spawn する UI の設計 → [tickets/tui-pod-spawn-ui.md](tickets/tui-pod-spawn-ui.md)
|
||||
|
|
|
|||
|
|
@ -23,7 +23,9 @@ pub use controller::{PodController, PodHandle};
|
|||
pub use factory::{FactoryError, PodFactory};
|
||||
pub use notifier::Notifier;
|
||||
pub use hook::{Hook, HookEventKind, HookRegistryBuilder};
|
||||
pub use manifest::{PodManifest, PodManifestConfig, ProviderConfig, ProviderKind, Scope};
|
||||
pub use manifest::{
|
||||
PodManifest, PodManifestConfig, PodMetaConfig, ProviderConfig, ProviderKind, Scope,
|
||||
};
|
||||
pub use pod::{Pod, PodError, PodRunResult, apply_worker_manifest};
|
||||
pub use prompt_loader::PromptLoader;
|
||||
pub use protocol::{ErrorCode, Event, Method, TurnResult};
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ use std::path::PathBuf;
|
|||
use std::process::ExitCode;
|
||||
|
||||
use clap::Parser;
|
||||
use pod::{Pod, PodController, PodFactory};
|
||||
use pod::{Pod, PodController, PodFactory, PodManifestConfig, PodMetaConfig};
|
||||
use session_store::FsStore;
|
||||
|
||||
#[derive(Parser)]
|
||||
|
|
@ -57,28 +57,17 @@ fn default_runtime_dir() -> Result<PathBuf, std::io::Error> {
|
|||
}
|
||||
}
|
||||
|
||||
/// Turn CLI inputs into a single programmatic overlay TOML string,
|
||||
/// combining `--pwd` and `--overlay`. Returns `None` if neither flag
|
||||
/// is set.
|
||||
fn build_overlay_toml(pwd: Option<&PathBuf>, overlay: Option<&str>) -> Option<String> {
|
||||
let mut parts: Vec<String> = Vec::new();
|
||||
if let Some(pwd) = pwd {
|
||||
// Canonicalize the pwd shorthand here so relative CLI arguments
|
||||
// (e.g. `--pwd .`) turn into the absolute path required by the
|
||||
// manifest cascade.
|
||||
let absolute = std::fs::canonicalize(pwd).unwrap_or_else(|_| pwd.clone());
|
||||
parts.push(format!(
|
||||
"[pod]\npwd = \"{}\"\n",
|
||||
absolute.display().to_string().replace('\\', "\\\\")
|
||||
));
|
||||
}
|
||||
if let Some(overlay) = overlay {
|
||||
parts.push(overlay.to_string());
|
||||
}
|
||||
if parts.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(parts.join("\n"))
|
||||
/// Construct a programmatic overlay [`PodManifestConfig`] that carries
|
||||
/// `pod.pwd` derived from the `--pwd` shorthand. Relative CLI paths
|
||||
/// are canonicalized here so the cascade always sees an absolute path.
|
||||
fn pwd_overlay(pwd: &PathBuf) -> PodManifestConfig {
|
||||
let absolute = std::fs::canonicalize(pwd).unwrap_or_else(|_| pwd.clone());
|
||||
PodManifestConfig {
|
||||
pod: PodMetaConfig {
|
||||
pwd: Some(absolute),
|
||||
..Default::default()
|
||||
},
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -103,9 +92,16 @@ async fn build_factory(cli: &Cli) -> Result<PodFactory, String> {
|
|||
.map_err(|e| format!("failed to auto-load project manifest: {e}"))?,
|
||||
};
|
||||
|
||||
if let Some(overlay) = build_overlay_toml(cli.pwd.as_ref(), cli.overlay.as_deref()) {
|
||||
// `--pwd` goes in as a typed config so path strings never have to
|
||||
// pass through TOML escaping. `--overlay` keeps its inline-TOML
|
||||
// interface (that is its entire reason for existing). Both feed
|
||||
// the same overlay slot and merge in call order.
|
||||
if let Some(pwd) = cli.pwd.as_ref() {
|
||||
factory = factory.with_overlay_config(pwd_overlay(pwd));
|
||||
}
|
||||
if let Some(overlay) = cli.overlay.as_deref() {
|
||||
factory = factory
|
||||
.with_overlay_toml(&overlay)
|
||||
.with_overlay_toml(overlay)
|
||||
.map_err(|e| format!("failed to parse overlay TOML: {e}"))?;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,198 +0,0 @@
|
|||
# Pod Factory: 設定カスケードとプロンプト資産による Pod 自動生成
|
||||
|
||||
## レビュー状態
|
||||
|
||||
初回レビュー実施済み。[pod-factory.review.md](pod-factory.review.md) を参照。
|
||||
要件全項目達成、アーキテクチャ・テスト被覆とも良好で**無条件で受け入れ可**。指摘は 6 件（すべて任意修正または nit レベル）。
|
||||
|
||||
## 背景
|
||||
|
||||
現状、Pod を起動するには `test_pod.local.toml` のような完全な `PodManifest` TOML を手書きする必要がある。1 人のユーザーが1 つのエージェントを試験運用するには十分だが、Insomnia が狙う「複数のエージェントが独立プロセスとして spawn されて自律的に動く」世界観では、**Pod のライフサイクル全体が自動化可能でなければならない**。そのためには、Pod の**作成自体**も自動化可能である必要がある。
|
||||
|
||||
手書きマニフェストには以下の問題がある:
|
||||
|
||||
- 1 Pod = 1 ファイルで、Pod を動的に増やす用途にスケールしない
|
||||
- 設定項目が多く（`worker.*` / `provider.*` / `scope.*` / `compaction.*` / `tool_output.*` 等）、毎回コピペしてわずかな差分だけ書き換える苦行になる
|
||||
- system_prompt を TOML 文字列に埋め込む形はプロンプト資産の再利用性が低い
|
||||
- Pod の起動条件の**共通部分**（プロバイダ・モデル・デフォルトツール設定など）は本来一度書けば良いのに、毎回書かされる
|
||||
|
||||
## ゴール
|
||||
|
||||
Pod 作成を「**最終的に `PodManifest` を1 つ構築する問題**」として定式化し、その `PodManifest` を**カスケード + 差分上書き**で組み立てる基盤を提供する。手書きが必要な TOML は「ユーザー / プロジェクト単位の**デフォルト上書き**」だけに縮退させ、個別の Pod 起動ごとに人間が TOML を触らない状態を目指す。
|
||||
|
||||
プロンプトは手書きマニフェストに文字列を埋め込む方式をやめ、**テンプレート資産ライブラリ**として参照可能にする。
|
||||
|
||||
## 方針
|
||||
|
||||
### 同じ型で、層で上書きする
|
||||
|
||||
- **解決後の型は現行の `manifest::PodManifest` のまま**（必須フィールド持ち）。
|
||||
- カスケードの各層は **`PodManifestConfig`**（`PodManifest` と同じ構造だが全フィールドを `Option` / 部分形で保持する新規型）で表現する。部分形同士を順番にマージし、最後に `TryFrom<PodManifestConfig> for PodManifest` で必須チェックを行いつつ確定させる。
|
||||
- 各層は**部分形**を持てる（全フィールドを埋める必要はない）。存在するフィールドだけが下層を上書きする。
|
||||
- 人間が書くときは `PodManifest` と同じ TOML スキーマで書く（サブセット可）。ファイル名は**すべて `manifest.toml`**。「設定」という曖昧な名前を避け、「Pod manifest」という語彙に揃える。
|
||||
|
||||
### カスケードの層
|
||||
|
||||
優先順位が低い方から高い方へ:
|
||||
|
||||
1. **ビルトインのデフォルト**: コードに焼き込んだ基本値（現在 `PodManifest` 各フィールドの `#[serde(default)]` や `Default` 実装に散っているものを集約）
|
||||
2. **ユーザー manifest**: `$XDG_CONFIG_HOME/insomnia/manifest.toml`（`XDG_CONFIG_HOME` 未設定時は `~/.config/insomnia/manifest.toml`）。ユーザー個人のプロバイダ指定・デフォルトモデル・常用ツール設定等を書く
|
||||
3. **プロジェクト manifest**: プロジェクトルート下の `.insomnia/manifest.toml`。プロジェクト固有の scope・compaction 設定・system_prompt のベース等を書く
|
||||
4. **プログラマティック overlay**: Pod 生成を呼ぶコード（GUI / CLI / 別 Pod からの spawn 等）が渡す部分形。ここで `pod.name` や `pod.pwd` のような**その Pod に固有の値**を与える
|
||||
|
||||
### プロジェクトルートの判定
|
||||
|
||||
起動ディレクトリから上方向に `.insomnia/` ディレクトリを探索し、**最も近い**ものをプロジェクトルートとする。見つからなければ「プロジェクト無し」として扱い、プロジェクト層をスキップする。
|
||||
|
||||
### マージのセマンティクス
|
||||
|
||||
- **スカラー** (`String`, `u32`, `bool` 等): 上層が存在すれば丸ごと置換
|
||||
- **Option 型**: 上層が `Some` なら置換、`None` なら据え置き
|
||||
- **マップ** (例: `tool_output.per_tool`): キー単位でマージ、同一キーは上層優先
|
||||
- **`scope.allow` / `scope.deny`**: **union（各層から全部足す）**。すべての層が追加のみ可能。最終的な effective scope は既存の `Scope::from_config` ロジックに委ね、`allow_union - deny_union` として解決される。**上位層は deny を追加することで下位層の allow を必ず削れる**ので、「上に行くほど強制力を持てる」構造が自然に出る
|
||||
- その他リスト: 当面は `scope.*` 以外にリスト型は存在しないが、新規にリスト型フィールドを追加するときは原則 union（scope と同じ）で扱う
|
||||
|
||||
### エラー戦略
|
||||
|
||||
- **未知フィールド**: TOML に書かれた未定義のキーは `tracing::warn!` のみ出して無視する。`#[serde(deny_unknown_fields)]` は**使わない**（将来バージョンアップで読めない旧設定が出るとユーザー体験が悪いため）
|
||||
- **型ミスマッチ**: `max_tokens = "100"` のような型エラーは hard error として resolve 失敗させる。ファイルパスと位置情報をエラーメッセージに含める
|
||||
- **パスフィールド**: `pod.pwd` / `provider.api_key_file` / `scope.*.target` 等のパスは**絶対パスのみ受け付ける**。相対パスが書かれていたら resolve 時に hard error とする。相対パス解決を層ごとの manifest_dir に依存させるとカスケードの意味論が濁るため、各ファイルを書く人間の責任で絶対パスを指定させる
|
||||
|
||||
### プロンプト資産ライブラリ
|
||||
|
||||
- プロンプトは TOML 文字列ではなく**ファイルとして管理**する。
|
||||
- 検索パスは以下の3層（上層優先で解決）:
|
||||
1. **ビルトイン**: レポジトリ直下の `resources/prompts/` 以下を `include_dir!` マクロでバイナリに同梱
|
||||
2. **ユーザー**: `$XDG_CONFIG_HOME/insomnia/prompts/`
|
||||
3. **プロジェクト**: `<project>/.insomnia/prompts/`
|
||||
- **ファイル名の stem がそのままプロンプト名**。サブディレクトリも許容し、`resources/prompts/common/tool-usage.md` は `{% include "common/tool-usage" %}` で参照できる
|
||||
- ファイル形式は **`.md`**（frontmatter は無し。前方互換として `---` 区切りの TOML フロントマターを将来持てる余地だけ認識しておく）
|
||||
- 既存の `SystemPromptTemplate`（minijinja ベース）の `Environment` にカスタムローダを仕込み、テンプレート内から他のプロンプトを `{% include "name" %}` / `{% import "name" as p %}` で参照できるようにする
|
||||
- プロンプト資産自体もテンプレートとして評価され、現行の `SystemPromptContext`（`now` / `cwd` / `scope` / `tools` / `files` 等）と同じ変数が見える
|
||||
- include 時の**変数伝搬は minijinja のデフォルト挙動**（親スコープの変数が自動で見える）に任せる
|
||||
|
||||
### 設定値のテンプレート参照は扱わない
|
||||
|
||||
`worker.max_tokens = "{{ env.INSOMNIA_MAX_TOKENS }}"` のような**設定値の中でテンプレートを展開する機能は本チケットの範囲外**とする。テンプレートエンジンはプロンプト本文の組み立てだけに限定する。設定値の動的化が必要になった時点で別チケットで検討する。
|
||||
|
||||
### プログラマティック Pod 作成 API
|
||||
|
||||
factory は `PodManifest` を組み上げる builder として設計する。イメージ:
|
||||
|
||||
```rust
|
||||
let manifest: PodManifest = PodFactory::new()
|
||||
.with_user_manifest_auto()? // XDG から自動読み込み、不在 OK
|
||||
.with_project_manifest_auto()? // cwd から上方向に .insomnia/ を探索、不在 OK
|
||||
.with_overlay_toml(overlay)? // programmatic な最上層 overlay（TOML 文字列）
|
||||
.resolve()?; // -> PodManifest
|
||||
Pod::from_manifest(manifest, store).await?;
|
||||
```
|
||||
|
||||
`Pod::from_manifest` は現在 `manifest_dir` を取るが、本チケットで以下の二段に書き換える:
|
||||
|
||||
- **`Pod::from_manifest(manifest: PodManifest, store)`** — 一次 API。factory の出力がそのまま入る
|
||||
- **`Pod::from_manifest_toml(toml: &str, store)`** — 便利関数。単層 manifest を TOML 文字列で直接投げる用途（テスト・デバッグ・動作確認）
|
||||
|
||||
`manifest_dir` 引数は廃止する。パスは絶対化済み前提のため、Pod 側で相対パス解決ロジックを持たない。ファイル読み込みは呼び出し側（factory か caller）の責務。
|
||||
|
||||
### CLI
|
||||
|
||||
`pod` バイナリの CLI surface を factory に合わせて更新する。新 umbrella コマンド（`insomnia spawn` 等）は**作らない**。ユーザーが直接 CLI を叩くのは debug/automation の文脈で、GUI/TUI は Pod を subprocess として spawn する運用のため、`pod` バイナリ1本の fla だけで十分。
|
||||
|
||||
```
|
||||
pod [--user-manifest <path>] [--project <path>] [--overlay <toml>] [--pwd <path>]
|
||||
```
|
||||
|
||||
- `--user-manifest`: 省略時は XDG から自動解決
|
||||
- `--project`: 省略時は cwd から上方向に `.insomnia/` を探索
|
||||
- `--overlay`: 最上層の overlay を inline TOML 文字列で渡す（例: `--overlay 'pod.name = "dbg"'`）
|
||||
- `--pwd`: 便利ショートカット（overlay に `pod.pwd = ...` を書く代わり）
|
||||
- 旧 `--manifest <path>` フラグは廃止。単層 manifest を直接読み込みたい場合は `--user-manifest <path>` で代替する
|
||||
|
||||
## 要件
|
||||
|
||||
### カスケード基盤
|
||||
|
||||
- ユーザー manifest・プロジェクト manifest・プログラマティック overlay を順に重ねた結果が `PodManifest` として取れる。
|
||||
- 各層が部分形を許容する（`pod.pwd` だけ書いてあっても良い等）。
|
||||
- マージセマンティクスがフィールドごとに定義され、単体テストで担保される（スカラー / Option / マップ / scope union）。
|
||||
- 未知フィールドは warn のみ、型ミスマッチは hard error。
|
||||
- 層が全て空で resolve 失敗する場合は、必要なフィールド（`pod.pwd` / `provider` / `scope.allow` 等）を明示するエラーを返す。
|
||||
|
||||
### プロンプト資産ライブラリ
|
||||
|
||||
- 3層の検索パスでプロンプトファイルを解決できる。
|
||||
- 同名プロンプトは上層優先で解決される。
|
||||
- `SystemPromptTemplate` の minijinja `Environment` にカスタムローダを仕込み、`{% include "name" %}` / `{% import "name" as x %}` で資産を参照できる。
|
||||
- 親テンプレートの変数が include 先にも見える（minijinja デフォルト挙動）。
|
||||
- ビルトインプロンプトは `resources/prompts/` から `include_dir!` マクロで同梱され、追加・編集するときは該当ディレクトリにファイルを置く・編集するだけで済む。
|
||||
- プロンプト資産自体もテンプレートとして評価され、`SystemPromptContext` と同じ変数が見える。
|
||||
|
||||
### プログラマティック Pod 作成
|
||||
|
||||
- `Pod::from_manifest(manifest: PodManifest, store)` と `Pod::from_manifest_toml(toml: &str, store)` の二段構成。path 受け API は廃止。
|
||||
- `PodFactory` builder が上記に繋がる。
|
||||
- TUI / GUI / daemon 等の上位クライアントが、TOML ファイルパスではなく overlay + 設定ディレクトリパスを渡すだけで Pod を起動できる。
|
||||
|
||||
### CLI
|
||||
|
||||
- `pod` バイナリが `--user-manifest` / `--project` / `--overlay` / `--pwd` を受け付ける。
|
||||
- 引数無しで cwd + XDG を自動解決して起動できる最小構成が動く。
|
||||
|
||||
### ドキュメント
|
||||
|
||||
- カスケード層の優先順位・マージ規則を `docs/` にまとめる。
|
||||
- ユーザー manifest / プロジェクト manifest の最小例と全オプション例を残す。
|
||||
- `resources/prompts/` のディレクトリ構造とビルトインプロンプト一覧を `docs/` に記載。
|
||||
|
||||
## リソース構成
|
||||
|
||||
```
|
||||
insomnia/
|
||||
├── crates/
|
||||
│ ├── pod/
|
||||
│ │ └── src/
|
||||
│ │ ├── factory.rs # 新規。PodManifestConfig カスケード → PodManifest
|
||||
│ │ ├── prompt_loader.rs # 新規。minijinja loader + 3層検索
|
||||
│ │ └── ...
|
||||
│ └── ...
|
||||
├── resources/
|
||||
│ └── prompts/
|
||||
│ ├── coder.md
|
||||
│ ├── reviewer.md
|
||||
│ ├── planner.md
|
||||
│ └── common/
|
||||
│ └── tool-usage.md
|
||||
```
|
||||
|
||||
- factory とプロンプト loader は新規 crate を作らず `pod` crate 内に配置する。既に `pod` が `manifest` に依存しているため追加の依存関係整理が不要
|
||||
- `include_dir!("$CARGO_MANIFEST_DIR/../../resources/prompts")` で全ビルトインプロンプトをバイナリに取り込む
|
||||
- ファイルの追加・編集は通常の file 操作だけで済む。既存ファイルの編集は cargo の変更追跡で自動再ビルドされる。新規追加・削除が反映されない場合は `cargo clean -p pod` を案内する（必要が増えたら `build.rs` で `rerun-if-changed` を足す）
|
||||
- ビルトインプロンプトの初期ラインナップ（`coder` / `reviewer` / `planner` / `common/tool-usage` 等）は実装時に選定し、ドキュメントに列挙する
|
||||
|
||||
## 完了条件
|
||||
|
||||
- `PodFactory` がカスケード層のマージで `PodManifest` を構築でき、マージセマンティクスの単体テストが通る（スカラー・Option・マップ・scope union・未知フィールド warn・型ミスマッチ hard error）。
|
||||
- ユーザー manifest・プロジェクト manifest・プログラマティック overlay のすべての層を使う end-to-end テストで、Pod が path を一切渡さずに起動できる。
|
||||
- プロンプト資産ライブラリを経由して system_prompt が組み立てられ、`{% include "ビルトイン名" %}` で `resources/prompts/` 以下の資産を参照できることをテストで確認できる。
|
||||
- `Pod::from_manifest(manifest)` と `Pod::from_manifest_toml(toml)` の二段 API が動き、path 受け API は削除されている。
|
||||
- `pod` バイナリが `--user-manifest` / `--project` / `--overlay` / `--pwd` を受け、引数無しでも cwd + XDG 自動解決で起動できる。
|
||||
- カスケード層の優先順位・マージ規則 / ユーザー manifest 例 / プロジェクト manifest 例 / ビルトインプロンプト一覧のドキュメントが `docs/` に存在する。
|
||||
|
||||
## 他チケットとの関係
|
||||
|
||||
- `tickets/native-gui-mvp.md`: 現状「manifest ファイルを選ぶ UI」を含むが、本チケット完了後はその UI が「overlay 指定 + Pod 起動」に置き換わる想定。native-gui-mvp 実装時に本チケットの API を使うか、先に path 渡しで済ませて後から差し替えるかは別途判断
|
||||
- `tickets/tui-pod-spawn-ui.md`: 同上。Pod spawn UI は本チケットが提供する API の上に構築される
|
||||
- `tickets/protocol-design.md`: Pod ↔ Client protocol 自体は変わらない。spawn 要求を protocol に載せるかどうかは protocol-design 側で検討
|
||||
- `docs/system-prompt-template.md` / `crates/pod/src/system_prompt.rs`: プロンプト資産ライブラリはこの minijinja 基盤の拡張として実装される
|
||||
|
||||
## 範囲外
|
||||
|
||||
- **preset の概念**: 名前付き overlay セット（`insomnia spawn coder` 等）は本チケットでは導入しない。将来別チケットで検討
|
||||
- **設定値の中のテンプレート展開**（`max_tokens = "{{ env.X }}"` のような動的値）。プロンプト本文のテンプレート展開のみを扱う
|
||||
- **GUI 内での manifest 編集 UI**。編集は人間がエディタで TOML を書くだけ（あくまで「Pod 生成時に手書きしない」ことを目指す）
|
||||
- **チーム共有・同期**。ユーザー manifest とプロジェクト manifest は各自・各リポジトリ単位で管理される
|
||||
- **秘密情報管理**（API キー等）。既存の `api_key_file` 方式を維持する
|
||||
- **設定値の型バリデーション強化**（JSON Schema など）。現行の serde ベースで十分な範囲に留める
|
||||
- **プロンプトフロントマター**（description / required_vars 等のメタデータ）。将来一覧 UI が必要になった時点で検討。フォーマット上の前方互換性だけは意識する
|
||||
- **umbrella CLI コマンド**（`insomnia spawn` 等）。`pod` バイナリ単体で完結させる
|
||||
|
|
@ -1,222 +0,0 @@
|
|||
# レビュー: Pod Factory
|
||||
|
||||
対象差分: `crates/manifest/src/{lib,config}.rs`, `crates/pod/src/{lib,pod,main,system_prompt,factory,prompt_loader}.rs`, `crates/pod/examples/{pod_cli,pod_protocol}.rs`, `crates/provider/src/lib.rs`, `resources/prompts/`, `docs/pod-factory.md`（いずれも未コミット）
|
||||
|
||||
## 要件達成状況
|
||||
|
||||
| 要件 | 状態 |
|
||||
|---|---|
|
||||
| カスケード基盤（ユーザー・プロジェクト・overlay の層マージ） | ✅ `manifest::PodManifestConfig` が部分形として実装。`PodFactory` が 4 層（default/user/project/overlay）を順にマージ |
|
||||
| 解決後の型は `PodManifest` のまま | ✅ `TryFrom<PodManifestConfig> for PodManifest` が検証付き変換 |
|
||||
| 各層の manifest.toml スキーマは同じ | ✅ partial 型群 (`PodMetaConfig` / `ProviderConfigPartial` / `WorkerManifestConfig` / ...) が本物と同構造 |
|
||||
| ユーザーパスは XDG | ✅ `user_manifest_path` が `XDG_CONFIG_HOME` → `$HOME/.config` の順 |
|
||||
| プロジェクトルートは `.insomnia/` 最近接 | ✅ `find_project_manifest` が cwd から上方向に walk |
|
||||
| scope は union マージ | ✅ `merge_scope` が allow/deny を `extend` |
|
||||
| マップは key-wise マージ | ✅ `ToolOutputLimitsPartial::merge` |
|
||||
| スカラー / Option は upper 優位 | ✅ `upper.x.or(self.x)` パターンで実装 |
|
||||
| 未知フィールドは warn のみ | ✅ `serde_ignored::deserialize` + `tracing::warn!` |
|
||||
| 型ミスマッチは hard error | ✅ toml パースで通常通り失敗 |
|
||||
| パスフィールドは絶対パスのみ | ✅ `ensure_absolute` が `pod.pwd` / `provider.api_key_file` / `scope.*.target` を検証 |
|
||||
| `Pod::from_manifest(manifest, store, loader)` 二段 API | ✅ + `from_manifest_toml` 便利関数。旧 path 受けは廃止 |
|
||||
| `manifest_dir` 引数の消滅 | ✅ `Pod` 構造体から削除、`provider::build_client` からも消滅 |
|
||||
| プロンプト3層ローダ | ✅ `PromptLoader` が project/user/builtin の順で解決 |
|
||||
| ビルトインは `resources/prompts/` + `include_dir!` | ✅ `pod/src/prompt_loader.rs:17` |
|
||||
| プロンプト include の親変数伝搬 | ✅ minijinja デフォルト挙動、`include_resolves_builtin_prompt` テストで確認 |
|
||||
| CLI の `--user-manifest` / `--project` / `--overlay` / `--pwd` | ✅ `pod/src/main.rs` で実装。引数無しでも XDG + cwd 自動解決で動く |
|
||||
| 引数無しでの最小構成動作 | ✅ CLI デフォルトが auto 系メソッドにつながる |
|
||||
| ドキュメント (`docs/pod-factory.md`) | ✅ カスケード層・マージ規則・CLI・プログラマティック API・ビルトイン一覧を網羅 |
|
||||
|
||||
**すべての要件を達成**。さらに当初のチケットに無かった点として CLI docs の整理、`examples/` の追随更新、provider の `~` 展開廃止（絶対パス徹底）までカバーしている。
|
||||
|
||||
## アーキテクチャ統合
|
||||
|
||||
### クレート境界
|
||||
|
||||
- **`manifest/src/config.rs`** — `PodManifestConfig` を manifest crate 側に置いたのは**正しい判断**。manifest crate は純データ型と検証ロジック (`Scope::from_config`, `TryFrom`) の置き場で I/O を持たない、という既存方針をそのまま継承している
|
||||
- **`crates/pod/src/factory.rs`** / **`prompt_loader.rs`** — I/O（ファイル読み、XDG 解決、`include_dir!`）は pod crate 側に集約。新規 crate を作らず、既存レイヤに素直に乗せている。前回私が推した統合方針どおり
|
||||
- **`PodFactory::resolve()` が `(PodManifest, PromptLoader)` を返す** — factory は「manifest + どこからプロンプトを引くか」を一体で管理し、Pod は渡された loader を素直に使うだけ。責務の分離が綺麗
|
||||
- **`Pod::manifest_dir` の完全削除 + `resolve_pwd` の絶対パス強制** — 「パスの正規化は cascade 層の専売事項」という単一ソースの原則が守られている。`provider::build_client` からも `manifest_dir` 引数が消え、`~` 展開も消え、relative rejection だけ残った。下流が大幅にシンプルになった
|
||||
|
||||
### 副作用的な改善
|
||||
|
||||
- `Pod::from_manifest_toml` が `PromptLoader::builtins_only()` を暗黙に使う → テスト・examples が「単層 TOML で Pod 起動」を一行で書けるようになった
|
||||
- `examples/pod_cli.rs` と `pod_protocol.rs` が絶対パス化と新 API に追随済み。壊れ残りなし
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### 1. 🟢 `with_overlay_toml` / `with_overlay_config` の重ね合わせが「同じ層にマージ」
|
||||
|
||||
`factory.rs:123-139`:
|
||||
|
||||
```rust
|
||||
pub fn with_overlay_toml(mut self, toml: &str) -> Result<Self, FactoryError> {
|
||||
let config = PodManifestConfig::from_toml(toml).map_err(FactoryError::OverlayParse)?;
|
||||
self.overlay = Some(match self.overlay {
|
||||
Some(existing) => existing.merge(config),
|
||||
None => config,
|
||||
});
|
||||
Ok(self)
|
||||
}
|
||||
```
|
||||
|
||||
`with_overlay_*` を複数回呼ぶと、独立したレイヤにはならず**1 つの overlay スロットに逐次マージ**される（後勝ち）。これは CLI + 1 回のプログラマティック注入には十分だが、「複数の独立した overlay を priority order 付きで積みたい」ニーズには応えない。
|
||||
|
||||
**判断**: 現状の要件範囲内では問題なし。将来 preset や環境変数ベースの overlay を追加する場合に再検討。
|
||||
|
||||
**細かい付随点**: `factory.rs` のテスト `cascade_overlay_overrides_project_overrides_user` は名前が「overlay が project を override し、project が user を override する」と読めるが、実際には**全部 overlay スロットに積まれている**（user/project スロットは未使用）。テスト内コメントで断り書きはあるが、名前だけ見ると誤解されやすい。`cascade_priority_layer_ordering` のほうが本来のレイヤ順序を検証しているので、前者は `overlay_stacking_merges_in_place` などに改名するとより正確。
|
||||
|
||||
### 2. 🟢 "builtin defaults" 層の実体がゼロ
|
||||
|
||||
チケット方針:
|
||||
> ビルトインのデフォルト: コードに焼き込んだ基本値（現在 `PodManifest` 各フィールドの `#[serde(default)]` や `Default` 実装に散っているものを集約）
|
||||
|
||||
実装方針（`factory.rs` module doc）:
|
||||
> 1. **Builtin defaults** — in-code defaults, currently empty. Upper layers provide everything; `TryFrom<PodManifestConfig>` fills in per-field defaults
|
||||
|
||||
実際の builtin layer は `PodManifestConfig::default()`（全部 None）で、デフォルト値の適用は `TryFrom` 内の `.unwrap_or(ToolOutputLimits::default())` のように散在している。つまり**「散らばってる defaults を builtin layer に集約する」という元の目的は達成されていない**。
|
||||
|
||||
**判断**: 実運用上の挙動は同じ（ユーザーから見れば `PodManifest` の各フィールドが既定値を持つことに変わりなし）。チケットの表現 vs 実装の厳密な乖離だけで、受け入れ可否には影響しない。将来 defaults の一覧を可視化したくなった時点で builtin layer を実体化する余地を残しておけば OK。
|
||||
|
||||
### 3. 🟢 CLI `--pwd` の overlay 注入が文字列フォーマット経由
|
||||
|
||||
`main.rs:64-76`:
|
||||
|
||||
```rust
|
||||
parts.push(format!(
|
||||
"[pod]\npwd = \"{}\"\n",
|
||||
absolute.display().to_string().replace('\\', "\\\\")
|
||||
));
|
||||
```
|
||||
|
||||
生成した TOML 断片を `with_overlay_toml` に渡している。`\` は escape しているが `"` は escape していないため、`"` を含むパス（Linux では理論上あり得る）で壊れる。また `--overlay` 側と `--pwd` 側を join するときに両者の構文が衝突しないかも微妙。
|
||||
|
||||
**推奨**: 文字列を作らず、`PodManifestConfig` を直接構築して `with_overlay_config` で渡す形に書き換える:
|
||||
|
||||
```rust
|
||||
if let Some(pwd) = cli.pwd.as_ref() {
|
||||
let absolute = std::fs::canonicalize(pwd).unwrap_or_else(|_| pwd.clone());
|
||||
factory = factory.with_overlay_config(PodManifestConfig {
|
||||
pod: PodMetaConfig { pwd: Some(absolute), ..Default::default() },
|
||||
..Default::default()
|
||||
});
|
||||
}
|
||||
if let Some(overlay) = cli.overlay.as_deref() {
|
||||
factory = factory.with_overlay_toml(overlay)?;
|
||||
}
|
||||
```
|
||||
|
||||
型を経由するので escape 問題が消え、`--pwd` と `--overlay` の干渉も無くなる。
|
||||
|
||||
**判断**: 現実には問題が起きる可能性は極めて低いが、型経由のほうが筋が良い。**任意**。
|
||||
|
||||
### 4. 🟢 `resolve_provider` に dead code
|
||||
|
||||
`manifest/src/config.rs:218-237`:
|
||||
|
||||
```rust
|
||||
fn resolve_provider(
|
||||
cfg: ProviderConfigPartial,
|
||||
field_prefix: &'static str, // ← 使われていない
|
||||
kind_field: &'static str,
|
||||
...
|
||||
) -> Result<ProviderConfig, ResolveError> {
|
||||
let _ = field_prefix; // ← 明示的に捨てられている
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
`field_prefix` を取っているが関数内で使っていない（`let _ =` で捨てている）。将来エラーメッセージで `"missing field: {prefix}.kind"` のようにしたい意図だったと推察されるが、現在の `ResolveError::MissingField` は静的文字列を直接受けているので不要。
|
||||
|
||||
**判断**: 不要なので削除推奨。**任意**（実害なし、lint が効けば dead_code 警告が出るかも）。
|
||||
|
||||
### 5. 🟢 `~` 展開廃止は breaking change
|
||||
|
||||
`provider` から `~/.config/insomnia/keys/anthropic` のような `~` 始まりパスの展開処理が消えた。既存の手書き manifest にこの形式が書かれていると resolve で `RelativePath` エラーになる。
|
||||
|
||||
**判断**: チケットで「絶対パスのみ」と決めた結果であり、意図通り。`docs/pod-factory.md` でも「パスの絶対性」として明記されている。受け入れ可。
|
||||
|
||||
### 6. 🟢 `find_project_manifest` の canonicalize 失敗時フォールバック
|
||||
|
||||
`factory.rs:197-208` は `start.canonicalize()` が失敗したら raw path で walk を続ける。`.insomnia` 名前の検出は path 比較だけなので動作するが、shell が与えた相対パスが絶対化されないまま `dir.parent()` を続けると仕様上の最上位で止まる可能性。
|
||||
|
||||
**判断**: 実害小。canonicalize が失敗するケースは cwd が無効等で、そもそも Pod 起動前の別エラーに出るはず。**不問**。
|
||||
|
||||
## テスト
|
||||
|
||||
- `manifest/src/config.rs`: 14 ケース（resolve 成功 / 必須欠落 / 相対パス3種 / scalar merge / scope union / per_tool keywise / option struct / type mismatch / unknown field / partial layer / end-to-end cascade）
|
||||
- `pod/src/factory.rs`: 7 ケース（overlay only / 模擬レイヤ順 / 実レイヤ順 / walk-up / 無プロジェクト / loader 連携 / 必須欠落）
|
||||
- `pod/src/prompt_loader.rs`: 6 ケース（builtin 存在 / サブディレクトリ / 未知 / user override / project override / fallthrough）
|
||||
- `pod/src/system_prompt.rs`: 2 ケース（include 成功 / 未知 prompt）
|
||||
- `provider/src/lib.rs`: 既存テストを新シグネチャに追随 + 新規 relative rejection
|
||||
|
||||
**合計 30 ケース超**、各層の検証が丁寧。特に `resolve_produces_loader_with_project_prompts_dir` は factory + prompt_loader + system_prompt の**3 層を貫く end-to-end テスト**で、この変更でもっとも壊れやすい配線を lock-in している点が優秀。
|
||||
|
||||
## 結論
|
||||
|
||||
**無条件で受け入れ可**。要件は全項目達成、アーキテクチャ統合も筋が良く、テスト被覆も厚い。指摘はすべて任意修正または nit レベルで、受け入れ可否に影響しない。
|
||||
|
||||
任意修正として推せるのは:
|
||||
|
||||
- **指摘 3**（CLI `--pwd` の型経由化）が最も筋が良い改善。余力があれば
|
||||
- **指摘 4**（dead code）は数行で済む clean-up
|
||||
- **指摘 1 のテスト名**（`cascade_overlay_overrides_project_overrides_user` → `overlay_stacking_merges_in_place` 等）も小さな改善
|
||||
|
||||
上記はいずれも受け入れ後の別タスクとしても良い。
|
||||
|
||||
---
|
||||
|
||||
## フォローアップ差分 (2026-04-16)
|
||||
|
||||
レビュー後の追加作業として、指摘 #2（builtin defaults の実体ゼロ）を解消し、
|
||||
**デフォルト値のメンテナンス性向上**を目的とした集約リファクタを入れた。
|
||||
|
||||
### 変更内容
|
||||
|
||||
- **`crates/manifest/src/defaults.rs` 新設**: 全 manifest デフォルト値を
|
||||
`pub const` で宣言する単一の真実ソース
|
||||
- `TOOL_OUTPUT_MAX_BYTES`, `PRUNE_PROTECTED_TURNS`,
|
||||
`PRUNE_MIN_SAVINGS`, `COMPACT_RETAINED_TURNS`
|
||||
- **`crates/manifest/src/lib.rs`**: 既存の `default_*` fn 群を constants を
|
||||
返すだけの 1 行に縮小。`ToolOutputLimits::default()` /
|
||||
`CompactionConfig::default()` の serde `#[default = "..."]` 経路もすべて
|
||||
constants に収束
|
||||
- **`crates/manifest/src/config.rs`**:
|
||||
- `PodManifestConfig::builtin_defaults()` を追加（cascade の最下層として使う
|
||||
構築メソッド、constants 直接参照）
|
||||
- `TryFrom<PodManifestConfig> for PodManifest` が `ToolOutputLimits::default()`
|
||||
/ `CompactionConfig::default()` 経由をやめ、constants を直接 `unwrap_or`
|
||||
する belt-and-suspenders 形
|
||||
- 指摘 #4 の `resolve_provider` dead param (`field_prefix`) を削除
|
||||
- **`crates/pod/src/factory.rs`**:
|
||||
- `PodFactory::resolve` の base layer を `PodManifestConfig::default()` から
|
||||
`PodManifestConfig::builtin_defaults()` に切替。これで "builtin layer" が
|
||||
実体を持つ cascade 層として機能
|
||||
- 指摘 #1 のテスト名 `cascade_overlay_overrides_project_overrides_user` →
|
||||
`overlay_stacking_merges_in_place` にリネーム
|
||||
- **`crates/manifest/src/config.rs` テスト追加**:
|
||||
- `builtin_defaults_populates_tool_output_max_bytes`
|
||||
- `builtin_defaults_merged_into_minimal_resolves_with_defaults`
|
||||
|
||||
### 効果
|
||||
|
||||
- デフォルト値を変えるときの編集箇所が**1 ファイル 1 行**（`defaults.rs`）に。
|
||||
従来は `default_*` fn、`Default::default()` impl、`TryFrom` フォールバックの
|
||||
3 経路にそれぞれ値が書かれていたが、すべて `defaults::X` を参照する形に収束
|
||||
- チケット本文で当初意図されていた「builtin layer がデフォルト値を保持する」
|
||||
概念が `builtin_defaults()` + factory の base 層で実体化
|
||||
- `TryFrom` の fallback は残すので、`builtin_defaults()` を経由しない直接構築
|
||||
（テスト等）でも同じ既定値が保証される（belt-and-suspenders）
|
||||
|
||||
### テスト
|
||||
|
||||
全 ワークスペース テスト通過（manifest 45 / pod 71 を含む）。新規 2 ケース
|
||||
で `builtin_defaults` の挙動と constants の同一性を lock-in。
|
||||
|
||||
### 未処理
|
||||
|
||||
- **指摘 3**（CLI `--pwd` の型経由化）は未対応。現在の文字列 format 経由でも
|
||||
escape 問題が実際に起きる可能性は極めて低いため保留。必要になった時点で別
|
||||
フォローアップとする
|
||||
|
||||
この差分の後、指摘 #2 / #4 / #1（テスト名）は解消済み。受け入れ可否には
|
||||
変化なし（元々「無条件で受け入れ可」）。
|
||||
Loading…
Reference in New Issue
Block a user