プロジェクトManifestの相対基準の修正
This commit is contained in:
parent
88e29d7bbe
commit
4ec8f63482
|
|
@ -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. `<project>/.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<Path>,
|
||||
) -> Result<Self, FactoryError> {
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -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 (`<project>/.insomnia/manifest.toml`) の相対 = そのディレクトリ基準
|
||||
- 相対パスの基準は層ごとに決める
|
||||
- user manifest (`~/.config/insomnia/manifest.toml`) の相対 = そのファイルの親ディレクトリ
|
||||
- project manifest (`<project>/.insomnia/manifest.toml`) の相対 = **プロジェクトルート**（`.insomnia/` の親）。`target = "."` がワークスペース全体を指すように
|
||||
- overlay（インライン TOML、ファイル位置なし）の相対パスは **プロセスの cwd** 基準
|
||||
- builtin 層には manifest を埋め込んでいないので対象外
|
||||
|
||||
|
|
|
|||
64
tickets/manifest-path-resolution.review.md
Normal file
64
tickets/manifest-path-resolution.review.md
Normal file
|
|
@ -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（要判断）
|
||||
|
||||
**状況**: 実装は `<project>/.insomnia/manifest.toml` に `target = "."` と書いた場合、**`<project>/.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 を親 `<project>/` にする。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」ガイドは別タスク化可（本チケット範囲外扱い）
|
||||
|
|
@ -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 <PATH>` で受け取って保持している（既存）。
|
||||
|
||||
### 又貸しの親連鎖
|
||||
|
||||
`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<SpawnedPodRegistry>` を 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 の送受信を検証する単体テスト
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user