protocol拡張の実装(完了)
This commit is contained in:
parent
b7b315cd39
commit
225e1bf58e
1
TODO.md
1
TODO.md
|
|
@ -1,7 +1,6 @@
|
|||
- [ ] テスト設計 → [tickets/test-design.md](tickets/test-design.md)
|
||||
- [ ] ツール設計
|
||||
- [ ] Bash ツール (Permission 層と統合) → [tickets/bash-tool.md](tickets/bash-tool.md)
|
||||
- [ ] Protocol の設計 → [tickets/protocol-design.md](tickets/protocol-design.md)
|
||||
- [ ] パーミッション: パターンベースのツール実行制御 → [tickets/permission-extension-point.md](tickets/permission-extension-point.md)
|
||||
- [ ] Pod オーケストレーション
|
||||
- [ ] 動的 Scope 変更 → [tickets/dynamic-scope.md](tickets/dynamic-scope.md)
|
||||
|
|
|
|||
|
|
@ -1,152 +0,0 @@
|
|||
# Protocol の設計
|
||||
|
||||
## 背景
|
||||
|
||||
現状の Protocol (`Method` / `Event`) は最低限のストリーミングイベントのみ。
|
||||
機能が増えるにつれ、以下が不足している:
|
||||
|
||||
- Compact 発生時のクライアント通知
|
||||
- Permission の ask/reply フロー（permission-extension-point の段階 3）
|
||||
- セッション切り替え（compact 後の新 `session_id` 通知）
|
||||
- クライアント→Pod の制御拡張（設定変更等）
|
||||
|
||||
## 現状（調査結果）
|
||||
|
||||
### Method (Client → Pod)
|
||||
|
||||
`crates/protocol/src/lib.rs:11-32`
|
||||
|
||||
```
|
||||
Run { input } | Notify { message } | PodEvent(PodEvent)
|
||||
Resume | Cancel | Pause | Shutdown | GetHistory
|
||||
```
|
||||
|
||||
### Event (Pod → Client)
|
||||
|
||||
`crates/protocol/src/lib.rs:85-134`
|
||||
|
||||
```
|
||||
TurnStart/TurnEnd, TextDelta/TextDone,
|
||||
ToolCallStart/ArgsDelta/Done, ToolResult,
|
||||
Usage, RunEnd, Error, History, Notification, Shutdown
|
||||
```
|
||||
|
||||
### 既存の request-response の扱い
|
||||
|
||||
`crates/pod/src/socket_server.rs:96-113` の `GetHistory` だけが
|
||||
「socket 層で即応答」。他の Method は `handle.send(method)` で
|
||||
Controller に fire-and-forget。
|
||||
|
||||
Client (`crates/tui/src/client.rs`) は reader を単一の mpsc に流すだけで、
|
||||
応答と broadcast を区別しない。`GetHistory` は 1 回きりで競合しないため
|
||||
現状は動く。
|
||||
|
||||
### Compact の現状
|
||||
|
||||
`crates/pod/src/pod.rs:825` の `compact()` は新しい `SessionId` を戻すが、
|
||||
ネットワークに一切乗らない:
|
||||
|
||||
- mid-turn: `do_compact_and_resume` (685行) — 失敗時のみ `Notification::Error`
|
||||
- post-turn: `try_post_run_compact` (715行) — 失敗時のみ `Notification::Warn`
|
||||
|
||||
成功時は完全にサイレント。`Event::History` / `Greeting` に `session_id` は入っていないため、
|
||||
TUI は現在の session_id を認識できない。
|
||||
|
||||
## 決定事項
|
||||
|
||||
### 1. request-response の扱い — 今は足さない
|
||||
|
||||
- 現状 `GetHistory` が 1 回きりで競合しないため、新しい wire フィールドなしで動く。
|
||||
- 将来 Permission ask/reply などが入る時点で、**Method/Event 双方に `request_id: Option<String>` を追加**する方針を採る。既存実装は無視するだけで互換を保てる。
|
||||
- `Response` を別 enum にする案 (C) は採用しない:
|
||||
- 先例として `PodEvent` が `Method::PodEvent(PodEvent)` で Method 側に包まれており、第 3 の wire 型を増やす理由がない。
|
||||
- Reader の分岐が増え、`PodClient` の mpsc 設計を崩す。
|
||||
|
||||
今回は **ドキュメントとしての宣言のみ**。フィールドは足さない。
|
||||
|
||||
### 2. Compact イベント — 追加する
|
||||
|
||||
```rust
|
||||
Event::CompactStart
|
||||
Event::CompactDone { new_session_id: SessionId }
|
||||
Event::CompactFailed { error: String }
|
||||
```
|
||||
|
||||
- 発火点は `do_compact_and_resume` と `try_post_run_compact` の 2 箇所。
|
||||
`event_tx.send(...)` を start / 成功 / 失敗に挟むだけ。
|
||||
- 既存の `Notification::Error/Warn`（compactor source）はイベント側に移行してよいが、
|
||||
初期移行では重複して出しても害はない。先に Event を足し、Notification は後続チケットで整理。
|
||||
- Broadcast で全クライアントに通知（compact は Pod 自律発火で、特定リクエストへの応答ではない）。
|
||||
|
||||
### 3. session_id 通知 — CompactDone に含める
|
||||
|
||||
- `Event::CompactDone { new_session_id }` に載せる。
|
||||
- 汎用 `SessionChanged` は作らない（YAGNI — fork 等が実装される時まで判断を保留）。
|
||||
- TUI 側は現状 session_id を利用していないので、イベントを受け取るだけでよい（将来 GetHistory 再取得などに使う）。
|
||||
|
||||
### 5. wire 型 — `protocol` が `uuid::Uuid` を直接扱う
|
||||
|
||||
- `SessionId` のパースは `protocol` クレートの責務。`protocol/Cargo.toml` に
|
||||
`uuid = { workspace = true, features = ["serde"] }` を追加し、
|
||||
`Event::CompactDone { new_session_id: uuid::Uuid }` として型付けする。
|
||||
- `session-store::SessionId` は `uuid::Uuid` のエイリアスなので、Pod 側は変換なしで渡せる。
|
||||
- 文字列経由にはしない（wire を弱く型付けして嬉しいことがない）。
|
||||
|
||||
### 6. broadcast 手段 — Pod が `event_tx` を直接保持
|
||||
|
||||
- `Notifier` は `Event::Notification` の replay バッファ専用のまま残し、compact 系イベントは
|
||||
通さない（意味が噛み合わない）。
|
||||
- `Pod` に `event_tx: Option<broadcast::Sender<Event>>` を持たせ、
|
||||
`attach_notifier` と同じタイミングで Controller 側から渡す。
|
||||
- compact 発火点では `self.event_tx.as_ref().map(|tx| tx.send(...))` で直接流す。
|
||||
|
||||
### 7. late subscriber への再配信 — しない
|
||||
|
||||
- compact イベントはバッファせず、broadcast 時点で購読していないクライアントには届かない。
|
||||
- TUI は現状 session_id を使っていないため、接続後に直近の compact を知る必要がない。
|
||||
- 必要になった時点（fork や複数クライアント同時接続が現実になった時）で別チケットで buffer 化。
|
||||
|
||||
### 4. Permission ask/reply — 別チケットで実装
|
||||
|
||||
- permission-extension-point の段階 3 で追加する。
|
||||
- Pod → Client は `Event::PermissionRequest { id, tool, args }`、
|
||||
Client → Pod は `Method::PermissionReply { id, allow }` を想定。
|
||||
- ここで使う `id` は 1. の `request_id: Option<String>` パターンに従う。
|
||||
protocol-design 本チケットでは足さず、permission-extension-point 側で導入する。
|
||||
|
||||
## 本チケットで実装するもの
|
||||
|
||||
1. `protocol` クレートに `uuid = { workspace = true, features = ["serde"] }` を追加し、
|
||||
`Event::CompactStart` / `Event::CompactDone { new_session_id: uuid::Uuid }` /
|
||||
`Event::CompactFailed { error: String }` を追加する。
|
||||
2. `Pod` に `event_tx: Option<broadcast::Sender<Event>>` を持たせ、Controller 側から
|
||||
`Notifier` と同じタイミングで渡す。
|
||||
3. `crates/pod/src/pod.rs` の compact 発火 2 箇所（`do_compact_and_resume` /
|
||||
`try_post_run_compact`）で start / 成功（CompactDone） / 失敗（CompactFailed）を broadcast する。
|
||||
4. `crates/tui/src/app.rs` の `handle_pod_event` に 3 分岐を追加し、
|
||||
既存の `NoticeWarn` / `NoticeError` と同じ枠で最低限のテキストを表示する。
|
||||
- `[compact] starting`
|
||||
- `[compact] done (new session <uuid先頭8字>)`
|
||||
- `[compact error] <message>`
|
||||
5. テスト:
|
||||
- Event の JSON roundtrip（`protocol` クレート）: 3 バリアント + uuid の shape
|
||||
- 成功/失敗/mid-turn の各パスで Event が発行されることを確認する統合テスト（`pod` クレート）
|
||||
|
||||
## 本チケットで実装しないもの
|
||||
|
||||
- `request_id: Option<String>` フィールドの追加（将来、必要になったチケットで足す）
|
||||
- `Event::SessionChanged`（fork 実装時に再検討）
|
||||
- Notification と CompactFailed の重複整理（後続チケット）
|
||||
- Permission ask/reply の wire 定義（permission-extension-point 段階 3）
|
||||
|
||||
## 検討メモ（将来向け）
|
||||
|
||||
- Event の肥大化が気になってきたら、`Event::Stream(StreamEvent)` のようにカテゴリ分けしてネストする余地はある。ただし現状 15 バリアントで破綻していないので先送り。
|
||||
- Protocol のバージョニングは未着手。クライアントの互換性問題が顕在化した時点で `Event::Hello { protocol_version }` のような握手を追加する。
|
||||
- Broadcast channel の特性上、遅い client では drop が起き得る。現状はログのみだが、compact 等の重要イベントを落とすとまずいので将来は per-client queue への置き換えを検討する。
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve with follow-up
|
||||
- レビュー詳細: [./protocol-design.review.md](./protocol-design.review.md)
|
||||
- 日付: 2026-04-21
|
||||
|
|
@ -1,58 +0,0 @@
|
|||
# Review: Protocol の設計
|
||||
|
||||
対象コミット: working tree (未コミット)
|
||||
レビュー日: 2026-04-21
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
チケット「本チケットで実装するもの」1〜5 を実装と照合した。
|
||||
|
||||
1. **`protocol` クレートに `uuid` 追加 + 3 バリアント追加**
|
||||
- `crates/protocol/Cargo.toml:11` に `uuid = { version = "1.23.1", features = ["serde"] }` を追加済み。
|
||||
- `crates/protocol/src/lib.rs:138-149` に `Event::CompactStart` / `Event::CompactDone { new_session_id: uuid::Uuid }` / `Event::CompactFailed { error: String }` を追加。決定事項 5 の型方針（`uuid::Uuid` 直接）に一致。
|
||||
- ただしチケット本文は `uuid = { workspace = true, features = ["serde"] }` と書いているが、ルート `Cargo.toml` には `[workspace.dependencies]` が存在せず、そのままでは解決不能。既存の `session-store` と同じく直接バージョン指定にしているのは実装として合理的（コードベース流儀に整合）。チケット文言との齟齬は残っているので、どちらかを合わせるのが望ましい（後述）。
|
||||
2. **`Pod` が `event_tx` を保持 / Controller が `Notifier` と同タイミングで渡す**
|
||||
- `crates/pod/src/pod.rs:124-128` にフィールド追加、`attach_event_tx` は `pod.rs:358-361`。全 4 箇所のコンストラクタで `event_tx: None` 初期化済み（`pod.rs:185, 251, 1182, 1242`）。
|
||||
- `crates/pod/src/controller.rs:104-107` で `attach_notifier` の直後に `attach_event_tx(event_tx.clone())`。決定事項 6 通り。
|
||||
3. **compact 発火 2 箇所で start/成功/失敗を broadcast**
|
||||
- `do_compact_and_resume`: `pod.rs:711` (start) / `pod.rs:718` (done) / `pod.rs:726-728` (failed)。
|
||||
- `try_post_run_compact`: `pod.rs:757` (start) / `pod.rs:764` (done) / `pod.rs:770-772` (failed)。
|
||||
- 既存 `notify(...)` の失敗通知（Compactor Warn/Error）も残っており、決定事項 2 の「初期移行では重複して出しても害はない」に整合。
|
||||
4. **TUI `handle_pod_event` の 3 分岐**
|
||||
- `crates/tui/src/app.rs:192-214` で 3 バリアントを `NoticeWarn`/`NoticeError` 枠に追加。
|
||||
- 表示文言もチケット指定通り（`[compact] starting` / `[compact] done (new session <先頭8字>)` / `[compact error] <message>`）。
|
||||
5. **テスト**
|
||||
- protocol クレート: JSON roundtrip 3 本（`event_compact_start_roundtrip` / `event_compact_done_roundtrip` / `event_compact_failed_roundtrip`）— `new_session_id` の shape（ハイフン付き UUID 文字列）も `serde_json::Value` 経由で検証済み。
|
||||
- pod クレート統合テスト: `crates/pod/tests/compact_events_test.rs` に `post_run_compact_success_broadcasts_start_and_done` / `post_run_compact_failure_broadcasts_start_and_failed` の 2 本。
|
||||
- **mid-turn (`do_compact_and_resume`) のテストは欠落**。ファイル先頭コメントで「`send_event` ヘルパが共通のため inspection で担保」と明記しているが、チケット要件 5 は「成功/失敗/mid-turn の各パスで Event が発行されることを確認する統合テスト」と明示的に 3 パスを要求している。要件に対して実装側の判断で削ったことになるので Follow-up 以上。
|
||||
|
||||
`cargo build -p protocol -p pod -p tui` と `cargo test -p protocol --lib` / `cargo test -p pod --test compact_events_test` はローカルで通過。ビルドが通り機能が実行可能であるという全体不変条件は保たれている。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- **層分離**: `protocol` は wire 型に専念し、Pod 層は `broadcast::Sender<Event>` を保持して直接 push。`Notifier` は `Event::Notification` の replay 専用として残り、compact 系は通していない。決定事項 6/7 通りで、Notifier の責務を汚さない良い切り分け。
|
||||
- **send_event の書き方**: `pod.rs:370-374` の `fn send_event(&self, event: Event)` は `if let Some(tx) = self.event_tx.as_ref() { let _ = tx.send(event); }`。subscriber 不在時の `SendError` を潰す挙動（決定事項 7 の「late subscriber には届かない」）と一致。late subscriber への再配信をしないのも合意通り。
|
||||
- **依存追加規律**: `uuid` は `cargo add -p protocol uuid --features serde` で追加された形跡（バージョン番号 `1.23.1` が精密指定で入っている）。`Cargo.lock` の patch-level 更新も自然。`cargo add` 利用ポリシーを満たす。
|
||||
- **クレート命名・モジュール分割**: 新規クレート追加なし。`Pod` 本体への追加は `attach_event_tx` / `send_event` の 2 メソッド + 1 フィールドに留め、compact ロジックは既存関数内の差し込みに限定。機能別ファイル分離のポリシーを破っていない（今回は既存メソッド内への数行挿入なので妥当）。
|
||||
- **プロトコルの健全性**: wire path は `broadcast::Sender<Event>` → `socket_server::handle_connection` の `rx.recv()` → `writer.write(&event)` で既存と同じルート。新バリアントを足すだけで TUI まで透過的に届く（`crates/pod/src/socket_server.rs:83-91`）。
|
||||
- **代替案の妥当性**: `Notifier` に compact を相乗りさせる案は、「Notification は Warn/Error レベル付き replay バッファで意味が噛み合わない」ため避けた判断は正しい。`Notifier` は今後も compact 系を通さないことを前提にできるようになった。
|
||||
- **YAGNI 方針**: `SessionChanged` の一般化や `request_id` フィールドの先取り導入を見送り、compact に絞った最小変更。コードベースを歪めていない。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
- なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
- **mid-turn のテスト省略はチケット要件 5 に対する未達**（`crates/pod/tests/compact_events_test.rs:1-6` のファイル冒頭で省略理由を明記済み）。`do_compact_and_resume` は `just_compacted` フラグと `Box::pin` の async 再帰、`resume()` 呼び出しが挟まるため、`try_post_run_compact` とは制御フローが別物。共通化しているのは `send_event` のみで、発火点の有無・順序・失敗時の `notify + Err(e)` 伝播は別のテスト対象である。後続チケットに流してよい程度の軽い欠落だが、要件の文言通り 1 本（例: 成功時 + `PodError::CompactThrash` での挙動、または Yielded からの compact success）追加するのが望ましい。
|
||||
- **ticket 文言と Cargo.toml の表現齟齬**: ticket では `uuid = { workspace = true, ... }` と書いているのに、実装は直接バージョン指定（`session-store` と同じ書き方）。どちらが正解かはプロジェクト方針だが、ワークスペースに `[workspace.dependencies]` が未定義なので直接指定の方が現状整合する。チケットの文言を「`session-store` と同じく直接バージョン指定で追加する」に修正する、あるいは `[workspace.dependencies]` を用意して両方揃える、のどちらか。後続対応で構わない。
|
||||
- **CompactThrash 時は Start/Failed のいずれも発火しない**（`pod.rs:698-702`）。thrash 検出は compact を実行前に弾くので `CompactStart` が出ないのは正しいが、TUI 側は「compact が必要だったのに thrash で諦めた」ことを知る手段がない。現状は `PodError::CompactThrash` が `run` の Result として返り、その先で誰かがエラー Event 化する想定と思われるが、今回のスコープでは扱わなくてよい。将来 compact 周りの可視化を強化するときに再考を。
|
||||
- **`CompactDone` の短縮 UUID 表現** (`tui/src/app.rs:199-203`): `to_string().chars().take(8)` は UTF-8 上 ASCII なので実質 `&uuid_str[..8]` と等価。実害はないが、`new_session_id.as_simple().to_string()[..8].to_string()` か `format!("{new_session_id:.8}")` 相当にするとアロケーション 1 回で済む。nit に近い。
|
||||
|
||||
### Nits
|
||||
- `crates/protocol/src/lib.rs:138-149` の docstring が `CompactStart`/`CompactDone`/`CompactFailed` で個別に説明されていて読みやすい。`CompactStart` のコメントに「Broadcast to all clients; not replayed to late subscribers.」まで書いているので、決定事項 7 が wire-type 側に滲み出して次のレビューアの手助けになる。このレベルで継続されると良い。
|
||||
- `compact_events_test.rs:121-131` の `drain` ヘルパは `loop { match ... { Err(_) => break } }` で lagged/closed を区別していない。このテストでは channel 容量 64 で十分なので問題ないが、将来テストを増やす際には `try_recv` の `TryRecvError::Lagged(n)` を拾って assert するとより堅牢。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve with follow-up** — 要件 1〜4 と アーキテクチャ方針（決定事項 5/6/7）は全て達成。ビルド・テストも通過。要件 5 のうち mid-turn テストの省略がチケット文言に対してわずかに未達だが、インスツルメント自体は 1 箇所共通 (`send_event`) で呼び出し側の差分も小さいため blocker ではなく follow-up として処理可能。チケット側の `uuid` 依存表記（`workspace = true`）と実装の直接指定の齟齬も合わせて、どちらかを揃える後続対応を推奨する。
|
||||
Loading…
Reference in New Issue
Block a user