From 09e465d5835e8fa08dbd83a2b628623329bcf313 Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 4 May 2026 12:55:29 +0900 Subject: [PATCH] =?UTF-8?q?feat:=20Pod=E3=81=AE=E3=82=B9=E3=83=86=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E3=82=B9=E5=90=8C=E6=9C=9F=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/controller.rs | 18 ++---- tickets/tui-pod-status-sync.md | 6 ++ tickets/tui-pod-status-sync.review.md | 80 +++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 tickets/tui-pod-status-sync.review.md diff --git a/crates/pod/src/controller.rs b/crates/pod/src/controller.rs index 82874823..5fd529c5 100644 --- a/crates/pod/src/controller.rs +++ b/crates/pod/src/controller.rs @@ -119,6 +119,12 @@ async fn finish_controller_run( St: Store, { if new_status == PodStatus::Busy { + // Surface the post-run busy window before kicking off the jobs so + // TUI / external observers see Busy regardless of whether the + // worker turn ended via success or error. Both branches in + // `run_with_cancel_support` return `PodStatus::Busy` for this + // path; emitting here keeps the two unified. + set_controller_status(shared_state, runtime_dir, event_tx, PodStatus::Busy).await; run_post_run_jobs(pod, alerter).await; } @@ -506,7 +512,6 @@ impl PodController { &event_tx, &cancel_tx, &shared_state, - &runtime_dir, ¬ify_buffer, self_parent_socket.as_ref(), &spawner_name, @@ -559,7 +564,6 @@ impl PodController { &event_tx, &cancel_tx, &shared_state, - &runtime_dir, ¬ify_buffer, self_parent_socket.as_ref(), &spawner_name, @@ -605,7 +609,6 @@ impl PodController { &event_tx, &cancel_tx, &shared_state, - &runtime_dir, ¬ify_buffer, self_parent_socket.as_ref(), &spawner_name, @@ -699,7 +702,6 @@ impl PodController { &event_tx, &cancel_tx, &shared_state, - &runtime_dir, ¬ify_buffer, self_parent_socket.as_ref(), &spawner_name, @@ -767,7 +769,6 @@ async fn run_with_cancel_support( event_tx: &broadcast::Sender, cancel_tx: &mpsc::Sender<()>, shared_state: &Arc, - runtime_dir: &RuntimeDir, notify_buffer: &NotifyBuffer, parent_socket: Option<&std::path::PathBuf>, self_name: &str, @@ -791,13 +792,6 @@ where PodRunResult::LimitReached => (PodStatus::Busy, RunResult::LimitReached), }; let _ = event_tx.send(Event::RunEnd { result: run_result }); - if status == PodStatus::Busy { - shared_state.set_status(PodStatus::Busy); - let _ = runtime_dir.write_status(shared_state).await; - let _ = event_tx.send(Event::Status { - status: PodStatus::Busy, - }); - } if matches!(run_result, RunResult::Finished) { crate::ipc::event::fire_and_forget( parent_socket.cloned(), diff --git a/tickets/tui-pod-status-sync.md b/tickets/tui-pod-status-sync.md index 9d57c08b..fd5d8d52 100644 --- a/tickets/tui-pod-status-sync.md +++ b/tickets/tui-pod-status-sync.md @@ -38,3 +38,9 @@ TUI 利用中、notice / alert / compact 表示が出た前後で `Ctrl-C` / `Ct - TUI の入力キューイング（`tickets/tui-input-queue.md`）。本チケットは状態同期と Ctrl 系操作の安定化に限定する。 - native GUI 側の状態管理。 - notice / alert の文言や見た目の全面 redesign。 + +## Review + +- 状態: Approve with follow-up +- レビュー詳細: [./tui-pod-status-sync.review.md](./tui-pod-status-sync.review.md) +- 日付: 2026-05-04 diff --git a/tickets/tui-pod-status-sync.review.md b/tickets/tui-pod-status-sync.review.md new file mode 100644 index 00000000..8f2bf23d --- /dev/null +++ b/tickets/tui-pod-status-sync.review.md @@ -0,0 +1,80 @@ +# Review: TUI Pod 状態同期と Ctrl 系操作の安定化 + +レビュー対象: `develop` の作業ツリー（staged + unstaged）。22 files / +641 -311。 + +## 前提・要件の確認 + +### 状態同期 (要件 1) +- **達成**。`PodStatus` を `protocol` クレートに昇格させ、`Event::Status` と `Event::History.status` を新設（`crates/protocol/src/lib.rs:317-328, 437-447`）。 + - GetHistory 応答に現在 status を埋め込み (`crates/pod/src/ipc/server.rs:162-168`)。 + - controller の status 遷移ごとに `Event::Status` を broadcast する単一経路を `set_controller_status` に集約 (`crates/pod/src/controller.rs:68-77`)。 + - TUI 側は `App.pod_status` を `Event::Status` / `Event::History.status` から駆動し、`set_pod_status` が `running` / `paused` / `busy` を一括更新する (`crates/tui/src/app.rs:110-118, 669-679`)。 +- attach 時の history 取得時点での status 同期は `attach_history_includes_current_status` テストでカバー (`crates/pod/tests/controller_test.rs:217-238`)。 + +### post-run 処理の扱い (要件 2) +- **達成**。`PodStatus::Busy` を新設し、`PodRunResult::Finished` / `LimitReached` で controller が一旦 Busy に入ってから post-run jobs (extract / consolidate / compact) を実行、完了後に Idle へ遷移する設計 (`crates/pod/src/controller.rs:79-135, 786-836`)。 +- Busy 中の `Event::Status::Busy` broadcast はテスト `run_end_enters_busy_until_post_run_finishes_and_broadcasts_status` で確認 (`crates/pod/tests/controller_test.rs:175-214`)。 +- TUI status line に `busy — finishing post-run work` 表示 (`crates/tui/src/ui.rs:880-890`)、Ctrl-X は Busy 中はガード (`crates/tui/src/main.rs:438-442`)。 +- `submit_input` も Busy 中は早期リターンし、誤って新しい turn を送信しない (`crates/tui/src/app.rs:298-302`)。 + +### Ctrl-C / Ctrl-X の分岐 (要件 4) +- **達成**。`crates/tui/src/main.rs:434-442, 581-595` で `app.pod_status` の値ごとに明示的に分岐: + - Ctrl-C: Running → Pause / それ以外 → 2-tap quit (TUI exit のみ、Pod は維持)。 + - Ctrl-X: Running → Cancel / Paused, Idle → Shutdown / Busy → ガード (押し戻し)。 +- 古い `running` / `paused` ローカルフラグも `pod_status` から派生するため二重情報源にはなっていない (`crates/tui/src/app.rs:110-118`)。 + +### Input polling (要件 3) +- **達成**。`run_loop` を3段階に再構成 (`crates/tui/src/main.rs:281-338`): + 1. Pod event を最大 32 件まで非ブロッキングでドレイン (バウンド付きで starvation 防止)。 + 2. `event::poll(ZERO)` で溜まった key event を全て処理、何かあれば再描画して即 `continue`。 + 3. (1)(2) 共に空ならばはじめて `select!` で blocking poll または `client.next_event` を待つ。 +- これにより Pod event 大量受信時でも各イテレーションで key 入力チェックが必ず走るため、Ctrl-C/Ctrl-X 取りこぼしは構造的に解消されている。 +- `client.try_next_event` (新設) はバウンドドレイン用 (`crates/tui/src/client.rs:38-40`)。 + +### テスト (要件 5) +- **達成**。`controller_test.rs` に 3本の状態遷移テストを追加: + - `run_end_enters_busy_until_post_run_finishes_and_broadcasts_status` (要件 2)。 + - `attach_history_includes_current_status` (要件 1)。 + - `pause_while_busy_is_idempotent_not_not_running` (Pause が Busy 中に NotRunning エラーを出さないこと)。 + +### CLAUDE.md コンテキスト加工原則 (観点 6) +- **違反なし**。`PodEvent` / `Notify` の系統は従来どおり `pod.push_notify(...)` で history append → 次の `pre_llm_request` で context へ流れる経路を踏襲 (`crates/pod/src/controller.rs:537, 683, 863, 883`)。 +- 状態 (Running / Busy 等) は context には差し込まれず、`Event::Status` / runtime_dir/status.json と Pod handle の `shared_state` のみで配信。 + +## アーキテクチャ・スコープ + +### 良い点 +- `PodStatus` が「Pod 内部状態」から「protocol レベルの値」へ昇格したのは妥当。`Event::Status` を明示すれば外側の TUI/GUI/CLI が同じ source of truth を読める。 +- post-run jobs の実行を `run_post_run_jobs` ヘルパに集約 (`crates/pod/src/controller.rs:79-108`)、各 method 分岐 (Run / Notify / Resume / PodEvent) に重複していたロジックを `finish_controller_run` 1本にまとめている。本チケット前は同じ post-run 三段階が4箇所に重複していたため、純粋なリファクタとしても健全。 +- Busy → Idle の遷移を `finish_controller_run` 内で完結させ、controller の outer loop は `run_with_cancel_support` の戻り値だけ見れば良い構造。 +- TUI input polling のリファクタは "drain bounded → poll terminal → fall back to select" の3段で、よくある正攻法。 + +### 範囲外との切り分け (観点 7) +- 入力キューイング (`tui-input-queue.md`) には踏み込んでいない。`tui-input-queue.md` は **"in-flight 中に submit された run の保留"** が主題で、本チケットは状態同期と入力 polling 構造のみ。サブミット中の Run は依然として `submit_input` の Busy ガード以外は controller 側 `AlreadyRunning` で拒否される (`crates/pod/src/controller.rs:464-470, 851-856`)。OK。 + +### 巻き込みの整理 (観点 8, 9) +- 確認した周辺ファイル (`crates/manifest/src/scope.rs`, `crates/memory/src/consolidate/{input,lock,tidy}.rs`, `crates/tools/src/task.rs`, `crates/session-metrics/src/lib.rs`, `crates/pod/src/compact/prune.rs`, `crates/pod/src/ipc/interceptor.rs`, `crates/pod/tests/{consolidation_test,pod_comm_tools_test,session_metrics_test}.rs`) はすべて **rustfmt 由来の表面整形のみ** で、セマンティクスは変わっていない。`git diff HEAD` で確認済み。 + - これらは本チケットでは不要だが、`cargo fmt` の副産物として一緒に乗ってしまったタイプ。レビュー的には "別 PR ならクリーン" だが、blocking ではない。 +- `crates/pod/src/lib.rs` の `pub use` 変更は `PodStatus` が `protocol` 側に移動したことに伴う必須の追従。 +- `crates/pod/src/runtime/dir.rs` の変更も同上 (test の import パスを `protocol::PodStatus` へ）。 +- `crates/pod/tests/consolidation_test.rs` 等の `+/-` も大半が rustfmt とインポートパス更新。 + +## 指摘事項 + +### Blocking +なし。 + +### Non-blocking / Follow-up +- **エラー経路の Status::Busy broadcast 抜け** — `run_with_cancel_support` の `Err(e)` 分岐 (`crates/pod/src/controller.rs:820-835`) は `(PodStatus::Busy, ...)` を返すが、成功側 (`crates/pod/src/controller.rs:794-800`) と違って `Event::Status::Busy` を明示的に broadcast していない。post-run jobs はエラー時にも走るので、その間 TUI 側の `pod_status` は最後に受信した `Running` のままで、Busy バッジは出ない。Idle 遷移は `finish_controller_run` で必ず broadcast されるので致命的ではないが、UX としては "エラー後の数秒間 status が Running のまま見える" 可能性がある。`finish_controller_run` の冒頭 (Busy だった場合) にもう一度 Status::Busy を打つ等の整合化を検討してよい。 +- **`runtime_dir.write_status` が Busy では呼ばれない** — 成功分岐 (`crates/pod/src/controller.rs:794-800`) は `shared_state.set_status` と event broadcast はやるが `runtime_dir.write_status` は呼ばない。disk 上の `status.json` を外部プロセスが読む用途では Busy が見えない。`Event::Status` を購読していない CLI / 監視スクリプトには影響しうる。本チケットの完了条件としては Event 経路が満たせていれば足りるので blocking ではない。 +- **rustfmt の巻き込み** — `crates/manifest/`, `crates/memory/consolidate/`, `crates/tools/task.rs`, `crates/session-metrics/`, `crates/pod/src/compact/prune.rs`, `crates/pod/src/ipc/interceptor.rs` 等の整形変更は本チケットの目的外。次回以降は事前に `cargo fmt` 単独でコミットするか、関係ないファイルへのフォーマット差分は除外することを推奨。今回は意味的に害はないので blocking ではない。 +- **attach 直後の初期 status は `Idle`** — `App::new` で `pod_status: PodStatus::Idle` を初期値に置く (`crates/tui/src/app.rs:89-108`)。`GetHistory` 応答が届く前 (数十ms) に Ctrl-C を押すと "Idle 扱い" の 2-tap quit 経路に流れる。実害は小さく要件 1 に違反しないが、コメントに記すか `History` 受信前は Ctrl 系を抑制する手もある。Follow-up 候補。 +- **post-run 中は method_rx が pump されない** — `finish_controller_run` 内で `run_post_run_jobs` を逐次 await しているため、その間 `Method::Run` / `Notify` / `Pause` / `Shutdown` は mpsc キューで待たされる。Pause/Shutdown を Busy 中に押した場合、後続の Idle 遷移後に処理される (Pause は idempotent no-op、Shutdown は遅れて反映)。要件は満たすが「Ctrl-X (Shutdown) 押下から数秒遅延しうる」点はドキュメント or follow-up 検討の余地。 + +### Nits +- `crates/pod/src/controller.rs:117-135` `finish_controller_run` の `new_status == PodStatus::Busy` 比較は2回出る。enum 拡張時の保守性のため、ローカル変数 `is_busy` で1回にまとめてもよい。 +- `crates/pod/src/ipc/interceptor.rs:18-22` の `tracing::warn` import 順は `cargo fmt` がやった整形だが、本チケットの差分から外せれば望ましい。 + +## 判断 + +**Approve with follow-up** — チケットの完了条件 (Running/Paused 同期、Busy の表示、starvation 解消、ユニットテスト) はすべて満たされており、設計の方向性 (PodStatus を protocol の一級型へ昇格、post-run jobs の集約) は健全。コードベースを歪めるような複雑化は見られず、範囲外の入力キューイングにも踏み込んでいない。エラー経路の Busy 表示と rustfmt 巻き込みは follow-up でフォローすればよく、blocking ではない。