From 4ec8f63482e67e828b2415e0f40631a9ae4cd2ec Mon Sep 17 00:00:00 2001 From: Hare Date: Sun, 19 Apr 2026 08:03:59 +0900 Subject: [PATCH] =?UTF-8?q?=E3=83=97=E3=83=AD=E3=82=B8=E3=82=A7=E3=82=AF?= =?UTF-8?q?=E3=83=88Manifest=E3=81=AE=E7=9B=B8=E5=AF=BE=E5=9F=BA=E6=BA=96?= =?UTF-8?q?=E3=81=AE=E4=BF=AE=E6=AD=A3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/pod/src/factory.rs | 51 +++++++++++------ tickets/manifest-path-resolution.md | 8 ++- tickets/manifest-path-resolution.review.md | 64 ++++++++++++++++++++++ tickets/pod-upstream-events.md | 43 +++++++++++++-- 4 files changed, 141 insertions(+), 25 deletions(-) create mode 100644 tickets/manifest-path-resolution.review.md diff --git a/crates/pod/src/factory.rs b/crates/pod/src/factory.rs index d91decdd..0b0614cd 100644 --- a/crates/pod/src/factory.rs +++ b/crates/pod/src/factory.rs @@ -14,12 +14,13 @@ //! //! Path resolution happens **before** merge. Each layer is resolved //! against its own base directory so that a relative `target = "."` -//! in the project manifest means "project directory" regardless of -//! how the user or overlay layers lay out their own paths: +//! in the project manifest means the project root regardless of how +//! the user or overlay layers lay out their own paths: //! //! - user manifest: base = the directory holding the manifest file -//! - project manifest: base = the directory holding the manifest file -//! (i.e. `/.insomnia/`) +//! - project manifest: base = the **project root** (the parent of +//! `.insomnia/`, not `.insomnia/` itself) so that natural project +//! manifests with `target = "."` cover the whole workspace //! - overlay: base = the process's `current_dir()` at the time the //! overlay is installed, since an inline TOML string has no file //! location of its own @@ -119,9 +120,7 @@ impl PodFactory { source, })?; if let Some(path) = find_project_manifest(&cwd) { - let base = manifest_base(&path)?; - self.project = Some((read_config_file(&path)?, base.clone())); - self.project_prompts_dir = Some(base.join("prompts")); + self.install_project_manifest(&path)?; } Ok(self) } @@ -133,13 +132,27 @@ impl PodFactory { start: impl AsRef, ) -> Result { if let Some(path) = find_project_manifest(start.as_ref()) { - let base = manifest_base(&path)?; - self.project = Some((read_config_file(&path)?, base.clone())); - self.project_prompts_dir = Some(base.join("prompts")); + self.install_project_manifest(&path)?; } Ok(self) } + /// Shared setup for `with_project_manifest_auto` / `_from`: record + /// the manifest's project root as the base for relative-path + /// resolution (the parent of `.insomnia/`, not `.insomnia/` itself) + /// so `target = "."` in a project manifest means the project root. + /// `prompts/` still lives inside `.insomnia/`. + fn install_project_manifest(&mut self, path: &Path) -> Result<(), FactoryError> { + let insomnia_dir = manifest_base(path)?; + let project_root = insomnia_dir + .parent() + .map(Path::to_path_buf) + .unwrap_or_else(|| insomnia_dir.clone()); + self.project = Some((read_config_file(path)?, project_root)); + self.project_prompts_dir = Some(insomnia_dir.join("prompts")); + Ok(()) + } + /// Install a programmatic overlay parsed from a TOML string. Any /// relative paths in the overlay are resolved against the process's /// current working directory at the time of this call — an inline @@ -535,11 +548,12 @@ permission = "write" } #[test] - fn project_manifest_relative_paths_resolve_against_insomnia_dir() { - // per ticket: base is the directory holding the manifest — - // `.insomnia/` for a project manifest. `target = "."` inside - // a project manifest therefore points at `.insomnia/`, not at - // the project root. + fn project_manifest_relative_paths_resolve_against_project_root() { + // `.insomnia/manifest.toml` is the marker for the project, but + // the intuitive base for its relative paths is the project + // root (the parent of `.insomnia/`) — `target = "."` in a + // project manifest should cover the whole workspace, not the + // `.insomnia/` subdir. let tmp = TempDir::new().unwrap(); let root = tmp.path().canonicalize().unwrap(); let insomnia_dir = root.join(".insomnia"); @@ -558,6 +572,10 @@ model = "m" [[scope.allow]] target = "." permission = "read" + +[[scope.allow]] +target = "src" +permission = "write" "#, ); @@ -566,7 +584,8 @@ permission = "read" .unwrap() .resolve() .unwrap(); - assert_eq!(manifest.scope.allow[0].target, insomnia_dir); + assert_eq!(manifest.scope.allow[0].target, root); + assert_eq!(manifest.scope.allow[1].target, root.join("src")); } #[test] diff --git a/tickets/manifest-path-resolution.md b/tickets/manifest-path-resolution.md index 6f709f2b..94d7fe65 100644 --- a/tickets/manifest-path-resolution.md +++ b/tickets/manifest-path-resolution.md @@ -1,5 +1,7 @@ # Manifest のパス解決: cwd ベース + manifest ファイル相対 +レビュー中: [manifest-path-resolution.review.md](manifest-path-resolution.review.md) + ## 背景 現状 manifest 内のパス（`pod.pwd` / `provider.api_key_file` / `scope.allow.target` / `scope.deny.target` / `compaction.provider.api_key_file`）は全て絶対必須で、相対パスは `ResolveError::RelativePath` で弾かれる。 @@ -15,9 +17,9 @@ cargo / pyproject / npm などに倣い「相対パスは manifest ファイル ## 新しい解決規則 - `pod.pwd` フィールドは削除。Pod の作業ディレクトリ = プロセスの cwd -- 相対パスは **manifest ファイルがあるディレクトリ** を基準に解決 - - user manifest (`~/.config/insomnia/manifest.toml`) の相対 = そのディレクトリ基準 - - project manifest (`/.insomnia/manifest.toml`) の相対 = そのディレクトリ基準 +- 相対パスの基準は層ごとに決める + - user manifest (`~/.config/insomnia/manifest.toml`) の相対 = そのファイルの親ディレクトリ + - project manifest (`/.insomnia/manifest.toml`) の相対 = **プロジェクトルート**（`.insomnia/` の親）。`target = "."` がワークスペース全体を指すように - overlay（インライン TOML、ファイル位置なし）の相対パスは **プロセスの cwd** 基準 - builtin 層には manifest を埋め込んでいないので対象外 diff --git a/tickets/manifest-path-resolution.review.md b/tickets/manifest-path-resolution.review.md new file mode 100644 index 00000000..842ec8b2 --- /dev/null +++ b/tickets/manifest-path-resolution.review.md @@ -0,0 +1,64 @@ +# Review: manifest-path-resolution + +実装コミット `aed46e6`（マニフェスト解決の相対パス化）に対するレビュー。`cargo build` clean、`cargo test --workspace` 全 pass。 + +## 総評 + +チケット要件（`pod.pwd` 削除、相対パス = manifest ファイル基準、overlay は cwd 基準、マージ前絶対化）を忠実に実装。テストもカスケード各層・不変条件違反・両 manifest レイヤの相対解決まで網羅。ビルド・テスト通過。 + +指摘 1 の UX 判断だけ合意したら完了可。 + +## 完了条件の対応 + +| 要件 | 状態 | 確認箇所 | +|---|---|---| +| `pod.pwd` 削除 | ✅ | `PodMeta` と `PodMetaConfig` から削除 | +| Pod pwd = cwd | ✅ | `pod.rs:current_pwd()` = `current_dir().canonicalize()` | +| 相対 = manifest ファイル基準 | ✅ | `PodManifestConfig::resolve_paths(base)` + `factory::manifest_base()` | +| overlay は cwd 基準 | ✅ | `factory::resolve_and_merge_overlay` で `current_dir()` を base | +| マージ前に絶対化 | ✅ | `factory::resolve()` で各層 `.resolve_paths(&base)` → merge | +| `ensure_absolute` は不変条件チェックのみ | ✅ | `TryFrom` に残存、cascade 層で通れば通るだけ | +| `--pwd` 廃止 | ✅ | CLI から削除 | +| `api_key_file = "keys/anthropic"` 動作 | ✅ | `resolve_paths_joins_relative_api_key_file` テスト | +| `scope.allow target = "."` 動作 | ✅ | `project_manifest_relative_paths_resolve_against_insomnia_dir` テスト | + +## 指摘と判断 + +### 1. project manifest の base が `.insomnia/` であることの UX（要判断） + +**状況**: 実装は `/.insomnia/manifest.toml` に `target = "."` と書いた場合、**`/.insomnia/`** を指す。チケットの「manifest ファイルのあるディレクトリ基準」を忠実に実装した結果。 + +```rust +// factory.rs:538 テスト +// project manifest 内 target = "." が .insomnia/ ディレクトリに解決される +assert_eq!(manifest.scope.allow[0].target, insomnia_dir); +``` + +**懸念**: ユーザが「project manifest で `target = "."`」と書いたら自然には「プロジェクトルート」を意図する。`.insomnia/` 下を scope の対象にしたい運用は稀。 + +**選択肢**: +- **(a)** このまま。規則の一貫性（全層 "manifest と同じ dir"）を優先し、ドキュメントで「`target = ".."` でプロジェクトルート」と案内 +- **(b)** project manifest のみ base を親 `/` にする。user manifest と挙動が揃わなくなる例外ルール +- **(c)** `.` 表記を使わず「絶対パス or `..` 表記」を案内のみ + +**判断**: **(a) を推奨**。user manifest (`$XDG_CONFIG_HOME/insomnia/`) で `keys/anthropic` が `$XDG_CONFIG_HOME/insomnia/keys/anthropic` を指すのと同じ規則で、シンプル。ただし project で違和感が強いので README / docs で**典型例のテンプレ**を提示する必要あり（例: `target = ".."` で project root）。 + +この判断を合意できれば完了条件を満たす。(b) を選ぶなら `factory.rs:manifest_base` から派生した project 専用の親ディレクトリ計算に差し替える小改修が必要（1 関数程度）。 + +### 2. `pod.pwd` を書いた古い manifest の警告が埋もれる + +**状況**: `from_toml_accepts_unknown_field` テストで確認されている通り、`pod.pwd = "/obsolete"` は `serde_ignored` の warn でスキップされる。`tracing_subscriber` が WARN 有効になっていないと出ない。 + +**判断**: 範囲外だが、移行ユーザが「設定したのに効かない」と混乱するリスクあり。README / CHANGELOG にマイ採用しないション注記を入れたい（本チケットに含めるか、別 issue を切るかは任意）。本チケットの完了条件には影響しない。 + +### 3. 軽微 + +- `PodManifestConfig::resolve_paths` の `debug_assert!(base.is_absolute())` は release で落ちないが、現状の呼び出し側（`manifest_base` / `current_dir`）が絶対を保証するので許容 +- `spawn_pod.rs` で overlay TOML から `pod.pwd` を消し、`Command::current_dir(spawner_pwd)` で子に cwd を伝える構造へ正しく移行 +- `Scope::from_config` の signature 変更（`base` 引数削除）が全呼出箇所に反映されている +- 不変条件違反（`ResolveError::RelativePath` / `ScopeError::RelativeTarget`）が両層でチェックされていて、万一 cascade 解決漏れがあっても catch される二重防衛になっている + +## 完了に向けた作業 + +- 指摘 1 について (a) で合意 → このままマージ +- ドキュメントでの「`target = ".."` で project root」ガイドは別タスク化可（本チケット範囲外扱い） diff --git a/tickets/pod-upstream-events.md b/tickets/pod-upstream-events.md index f6674c13..ac80b5a6 100644 --- a/tickets/pod-upstream-events.md +++ b/tickets/pod-upstream-events.md @@ -59,14 +59,28 @@ pub enum PodEvent { | タイミング | variant | |---|---| | `RunEnd { result: Finished }` 発行時 | `TurnEnded` | -| `Event::Error` 発行時 | `Errored` | +| Worker 実行エラー発生時（後述） | `Errored` | | shutdown シーケンス（controller loop 終了直前） | `ShutDown` | | `SpawnPod` tool 成功直後 | `ScopeSubDelegated` | +`Errored` の対象は **`run_with_cancel_support` 内で Worker 実行が失敗した `Event::Error`** に限定する。`AlreadyRunning` / `NotPaused` / `NotRunning` のような method 拒否応答はクライアント向けの一時的なフィードバックであり、親に通知すべき子のライフサイクルイベントではない。実装時は発火点を worker エラー経路に絞ること。 + 送信は非同期 spawn で発射し、await しない。接続失敗はログのみで続行（親が落ちていても子は生きる）。 親 socket のアドレスは spawn 時に `--callback ` で受け取って保持している（既存）。 +### 又貸しの親連鎖 + +`ScopeSubDelegated` は**直接の親にのみ送る**。曾孫が現れた場合： + +1. 孫 (C) が曾孫 (D) を `SpawnPod` する +2. C は自分の親 (B) に `ScopeSubDelegated { parent_pod: C, sub_pod: D, ... }` を送る +3. B は D を自分の `spawned_pods.json` に追加し、さらに B の親 (A) へ `ScopeSubDelegated { parent_pod: B, sub_pod: D, ... }` を再発射する + +これを再帰的に繰り返すことで、`scope.lock` の `delegated_from` チェーンと一致した形で root まで D の存在が登録される。各 Pod は「自分の直接の子＋紹介された孫（= 更に下の Pod も含む）」を把握し、孫より下の階層は子経由で管理される。 + +再発射は `ScopeSubDelegated` 受信時の system 処理の一部として行う（後述の表に反映）。 + ### 親（受信側） 親 Controller の main loop に `Method::PodEvent` ハンドラを追加： @@ -89,10 +103,20 @@ variant 別の (1) の中身： | `TurnEnded` | なし | | `Errored` | なし（LLM に判断させる） | | `ShutDown` | `spawned_registry.remove(pod_name)`、scope lock を flock して該当 allocation を `release_pod` で解放 | -| `ScopeSubDelegated` | `spawned_registry.add(SpawnedPodRecord { sub_pod, sub_socket, scope, ... })`（孫を直接把握する。親が子を経由せず孫を管理することで、子が死んでも孫の scope 管理が維持される） | +| `ScopeSubDelegated` | `spawned_registry.add(SpawnedPodRecord { sub_pod, sub_socket, scope, ... })` で孫を追加。続けて自分の親（いれば）へ `ScopeSubDelegated { parent_pod: self, sub_pod, ... }` を再発射（親連鎖参照） | (2) の `render_event` は一箇所に集約し、`format!("Pod `{pod_name}` finished a turn.")` のような短い human-readable 文字列を返す。 +### Controller の配線変更 + +現状 `PodController::spawn` 内で `SpawnedPodRegistry` は tool 登録のためだけに生成され、main loop の `method_rx` 処理ループには渡っていない。`Method::PodEvent` ハンドラを追加するには： + +- `spawned_registry: Arc` を main loop に `clone` で持ち込む +- `scope_lock::default_lock_path()` を event 処理時に呼ぶ（毎回 open するので保持は不要） +- 親 callback socket（自分の親、トップ Pod では `None`）を `Pod::from_manifest_spawned` 経由で既に保持している値から再利用（再発射で使う） + +`apply_event_side_effects` は Controller の main loop から呼ぶ非同期関数として定義する。 + ### 失敗時のフォールバック - 子 → 親の PodEvent 送信が失敗しても諦める（再試行しない） @@ -102,16 +126,23 @@ variant 別の (1) の中身： ## 設計で決めること - **送信の接続タイムアウト**: `SpawnPod` / pod-comm-tools と揃える（5 秒想定） -- **同時多発イベントの順序保証**: `TurnEnded` 直後に `ShutDown` が起きた場合、親側で順序を保証する必要があるか。現状は fire-and-forget で並列送信されうる -- **`ScopeSubDelegated` の親連鎖**: 孫がさらに曾孫を spawn したとき、曾孫の `ScopeSubDelegated` は誰に送る？（子に送り、子が親に転送? or 最上位の root まで届ける? or 直接の親だけで十分?） + +### 決定事項 + +- **順序保証は求めず、ハンドラを冪等・遅延到着に強くする**: fire-and-forget の unix socket 接続は順序を保証しない。`TurnEnded` 直後に `ShutDown` が届いても、逆順で到着しても親側で成立するように `apply_event_side_effects` を設計する。具体的には： + - `ShutDown` 受信時、すでに registry から削除済みでもエラーにしない（`release_pod` の `UnknownPod` を swallow する既存挙動を踏襲） + - `TurnEnded` が `ShutDown` より後に届いても、該当 Pod が既に registry にいなければ render だけして終える（LLM 向け通知は出る、system 処理は no-op） + - `ScopeSubDelegated` で孫が既に registry にいたら上書きせず no-op（`DuplicatePodName` を swallow） +- **`ScopeSubDelegated` の親連鎖は直接の親のみ + 再発射**: 上記「又貸しの親連鎖」セクション参照。曾孫以上は再帰的に再発射で root まで届く ## 完了条件 - `Method::PodEvent(PodEvent)` が `protocol` crate に追加され、serde round-trip テストが通る -- 子の Controller が 4 種の variant を適切なタイミングで親 socket に送信する +- 子の Controller が 4 種の variant を適切なタイミングで親 socket に送信する。`Errored` は Worker 実行エラーにのみ限定されることを確認 - 親の Controller が variant ごとの system 処理を実行し、レンダリングした文字列を LLM 通知バッファに流す -- `ScopeSubDelegated` 受信後、孫 Pod が親の `spawned_pods.json` に現れる +- `ScopeSubDelegated` 受信後、孫 Pod が親の `spawned_pods.json` に現れ、親が更に上位親を持つ場合は上位へ再発射される - `ShutDown` 受信後、該当 Pod が親の registry から消え、scope lock からも解放される +- イベント到着順が入れ替わっても副作用が安全（冪等）であることを単体テストで確認 - 送信失敗しても子プロセスが続行する - 各 variant の送受信を検証する単体テスト