From 89a66f1d58674f82d7d6716e7cad7c7206372a5d Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 4 May 2026 00:05:59 +0900 Subject: [PATCH] =?UTF-8?q?docs(tickets):=20tui=E3=83=88=E3=83=BC=E3=82=AF?= =?UTF-8?q?=E3=83=B3=E8=A1=A8=E7=A4=BA=E3=83=AC=E3=83=93=E3=83=A5=E3=83=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tickets/tui-upload-tokens-net.md | 5 ++++ tickets/tui-upload-tokens-net.review.md | 36 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 tickets/tui-upload-tokens-net.review.md diff --git a/tickets/tui-upload-tokens-net.md b/tickets/tui-upload-tokens-net.md index 2e4725f8..9b94da96 100644 --- a/tickets/tui-upload-tokens-net.md +++ b/tickets/tui-upload-tokens-net.md @@ -39,3 +39,8 @@ cache_creation は「このリクエストで新たにキャッシュに書い - 料金計算・課金表示の追加（あくまで「アップロード量」の意味付け修正）。 - セッション全体（複数ターン）の累積表示。 - cache hit 率のような追加指標の表示。 + +## Review +- 状態: Approve +- レビュー詳細: [./tui-upload-tokens-net.review.md](./tui-upload-tokens-net.review.md) +- 日付: 2026-05-03 diff --git a/tickets/tui-upload-tokens-net.review.md b/tickets/tui-upload-tokens-net.review.md new file mode 100644 index 00000000..8d01e6ad --- /dev/null +++ b/tickets/tui-upload-tokens-net.review.md @@ -0,0 +1,36 @@ +# Review: TUI ステータスライン `↑` をキャッシュ控除後の純アップロード量にする + +## 前提・要件の確認 + +- **protocol `Event::Usage` に `cache_read_input_tokens: Option` を追加**: `crates/protocol/src/lib.rs:284-297` で追加済み。`#[serde(default, skip_serializing_if = "Option::is_none")]` 付与でフォワード/バックワード互換を維持。doc コメント (`lib.rs:284-291`) で「占有量・キャッシュ subset・差分が net upload」という意味付けが明文化されており、ticket の要件記述（最低限 cache_read）に正確に合致。 +- **pod controller の passthrough**: `crates/pod/src/controller.rs:228-233` で `worker.on_usage` クロージャから `event.cache_read_input_tokens` をそのまま `Event::Usage` に流している。worker `UsageEvent` (`crates/llm-worker/src/llm_client/event.rs:67-79`) は元々全プロバイダで cache_read を normalize 済みなので、passthrough だけで正しい値が乗る。 +- **TUI 累積・表示**: `crates/tui/src/app.rs:469-483` で `input_tokens.unwrap_or(0).saturating_sub(cache_read_input_tokens.unwrap_or(0))` を `run_upload_tokens` に加算。`saturating_sub` 採用は cache_read が混在 normalize の都合で input より大きくなる病的ケースに対するガードとして正しい。`Event::RunEnd` (`app.rs:487-499`) でも同フィールドを `Block::TurnStats` に詰めて 0 リセット。`Block::TurnStats` レンダリング (`crates/tui/src/ui.rs:407-422`) と `draw_status` (`ui.rs:778-793`) も `upload_tokens` / `run_upload_tokens` を参照しており、表示文字列は元と同じ `↑{}/↓{}`。 +- **`pod_protocol` example**: `crates/pod/examples/pod_protocol.rs:76-87` でパターンに `cache_read_input_tokens` を追加し、`println!` に `cache_read=` を出している。ビルド整合に必要な最小変更。 +- **session-store 不変**: ticket 通り `LogEntry::LlmUsage` 周辺は触っていない。 +- **ビルド・テスト**: `cargo check -p protocol -p pod -p tui` 通過 (報告済 + こちらでも再確認、新規警告なし)。 + +完了条件「protocol/pod/TUI が cache_read 情報を受け渡せる経路ができている」「`Block::TurnStats` の `↑` も同じ意味で揃う」はいずれも満たされている。tool ループ実機検証は user 側マニュアルチェックの範囲だが、計算式自体は ticket の指定通り。 + +## アーキテクチャ・スコープ + +- 変更は **protocol → pod → tui** の縦方向 1 経路に閉じており、llm-worker (低レベル基盤) には触れていない。MEMORY.md の「llm-worker は低レベル基盤」原則に整合。 +- 新規クレートなし、依存追加なし。`cargo add` 関連メモは無関係。 +- protocol 変更は単一フィールド追加のみで、`Option` + `serde(default, skip_serializing_if)` により旧 Pod ↔ 新 TUI（フィールド欠落 → `None` → `saturating_sub(0)` で従来挙動）と新 Pod ↔ 旧 TUI（未知フィールドは serde 既定で無視）の両向きに非破壊。互換挙動の判断は妥当。 +- フィールド名リネーム (`run_input_tokens` → `run_upload_tokens`、`Block::TurnStats.input_tokens` → `upload_tokens`) は ticket の「フィールド意味も同方向に揃える」という指示を字面通り実装と命名の両方で揃えたもの。元の `input_tokens` 名は意味が変わったあと残すと将来の読み手を misleading するため、リネームしておく方がコードベース健全性として正しい判断。命名のみ変更で外部 API (protocol) には波及していない点も適切。 +- ticket 範囲外（料金計算・複数ターン累積・cache hit 率・TUI 以外のクライアント変更）への踏み込みなし。確認: `Event::Usage` を消費する他クレートは llm-worker 系内部の `LlmEvent::Usage(UsageEvent)`（別物）と pod_protocol example のみ。daemon/pod-cli/native-gui 等の追加クライアントは crate として未存在で巻き込みなし。 +- `cache_creation_input_tokens` は protocol に **乗せない** 判断: ticket 20 行目「cache_creation は full price 側として上載せされたまま」「表示式 `input - cache_read`」と完全に整合。worker `UsageEvent` には `cache_creation_input_tokens` が存在するが、現 protocol 公開層では誰も使わないので過剰公開を避けたのは適切（必要になったら同じパターンで足せる）。 + +## 指摘事項 + +### Blocking +なし。 + +### Non-blocking / Follow-up +- protocol の doc コメント (`crates/protocol/src/lib.rs:284-291`) に「`input_tokens` は cache 込み占有量である」旨が書かれているが、`output_tokens` と従来の `input_tokens` の意味（worker `UsageEvent` 規約と同じ）について、protocol 公開時点での規約として一段明示があると将来の他クライアント実装者が迷いにくい。今回の修正対象ではないので follow-up でよい。 +- `Block::TurnStats` の `output_tokens` フィールドは命名変更されていない（ticket 範囲外）。`upload_tokens` だけ「net」意味に変えたので、対称性を意識するなら将来 `output_tokens` も「output_tokens（cache 概念なし）」のままで OK だが、表示が `↑/↓` なので追加説明不要との判断は妥当。 + +### Nits +- `crates/tui/src/app.rs:474-477` のコメントは "cache_creation stays in (it is full price on this request)" と簡潔で分かりやすい。protocol 側の doc と相互参照しなくても閉じて読めるので維持で良い。 + +## 判断 +**Approve（完了可）** — ticket の要件・完了条件はすべて満たされ、互換性とアーキテクチャ境界を保ちながら最小差分で意味付けを修正している。リネームを伴うフィールド名変更も意味のドリフトを防ぐ正しい判断。