From be96efb5eddb93015f8f394b11cab4045b2f2695 Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 14 Apr 2026 02:57:25 +0900 Subject: [PATCH] =?UTF-8?q?prune-projection=E5=AE=8C=E4=BA=86?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- TODO.md | 1 - tickets/prune-projection.md | 91 --------------------- tickets/prune-projection.review.md | 122 ----------------------------- 3 files changed, 214 deletions(-) delete mode 100644 tickets/prune-projection.md delete mode 100644 tickets/prune-projection.review.md diff --git a/TODO.md b/TODO.md index 0e16db5d..263a2da9 100644 --- a/TODO.md +++ b/TODO.md @@ -2,7 +2,6 @@ - [ ] ツール設計 - [ ] Bash ツール (Permission 層と統合) → [tickets/bash-tool.md](tickets/bash-tool.md) - [ ] Scope の再設計 (pwd + writable、必須化) → [tickets/scope-redesign.md](tickets/scope-redesign.md) -- [ ] Prune をコンテキスト射影に変更 → [tickets/prune-projection.md](tickets/prune-projection.md) - [ ] Prune の savings 推定を正確にする → [tickets/prune-savings-estimation.md](tickets/prune-savings-estimation.md) - [ ] Compact の改善（要約品質 + 挙動詳細） → [tickets/compact-improvements.md](tickets/compact-improvements.md) - [ ] Protocol の設計 → [tickets/protocol-design.md](tickets/protocol-design.md) diff --git a/tickets/prune-projection.md b/tickets/prune-projection.md deleted file mode 100644 index e90f1ecb..00000000 --- a/tickets/prune-projection.md +++ /dev/null @@ -1,91 +0,0 @@ -# Prune をコンテキスト射影に変更 - -## 背景 - -現状の `apply_prune` は Worker が保持する history の `Item::ToolResult { content }` -を直接 `None` に書き換えている。PreLlmRequest hook の `context: &mut Vec` は -Worker の history そのものなので、prune すると元の content が永久に失われる。 - -問題: -- session-store に persist される history が prune 済みになり、restore しても content が戻らない -- compact worker が要約を作る際に、本来参照できるはずの content が消えている -- 「どこまで prune したか」という状態が暗黙的（content が None かどうか） - -## 方針 - -永続化された記録（Worker の history / session-store のログ）は変えず、 -LLM に送るコンテキストを組み立てる段階で content を省く。 - -Prune は「どの ToolResult の content を省略するか」を決める射影ロジックであり、 -history の変換ではない。 - -### 設計の方向 - -1. prune の判定結果を「省略対象の index 集合」として保持する - （現在の `prunable_indices` がそのまま使える） -2. PreLlmRequest hook でコンテキストを構築する際に、省略対象の - ToolResult は content を含めずに組み立てる -3. Worker の history は一切変更しない - -### 影響範囲 - -- `crates/llm-worker/src/prune.rs`: `apply_prune` を廃止または射影版に置き換え -- `crates/pod/src/prune_hook.rs`: history を mutate せず、射影したコンテキストを返す -- PreLlmRequest hook の戻り値: 現在 `PreRequestAction::Continue` で history を - そのまま使う設計。射影したコンテキストを渡す方法の検討が必要 - -## 追加作業: Worker への統合 - -初回レビューで「pure ロジックが `llm-worker::prune` にあるのに適用は pod 側 -Hook で行う」という責務の不整合が見つかった。現状: - -- `llm-worker::prune` が `PruneConfig` と `prunable_indices` を提供 -- `pod::prune_hook::PruneHook` が `PreLlmRequest` hook として射影を実行 - -Worker 自身が context window 管理の責任を持つべき。`build_request` の直前 -（`worker.rs:701` 付近）で射影すれば `PruneHook` は不要になる。 - -### 変更内容 - -- `Worker` が `PruneConfig` を直接保持する - （`Worker::set_prune_config(config)` 等） -- `worker.rs:701` の `request_context = self.history.clone()` の直後に - Worker 自身が `prunable_indices` で候補を抽出し、`min_savings` を満たせば - content を射影する -- `min_savings` 判定のためのトークン会計は Worker から外部に問い合わせる。 - `Worker::set_savings_estimator(Box) -> u64>)` - のようにコールバックを注入する形にする - （Worker は usage 履歴を知らないので、計算は pod 側に残す） -- `pod::prune_hook` モジュールを廃止。Pod は `worker.set_prune_config(...)` と - `worker.set_savings_estimator(...)` を呼ぶだけ - -### 影響範囲 - -- `crates/llm-worker/src/worker.rs`: `PruneConfig` / savings estimator の - 保持、`build_request` 直前での射影適用 -- `crates/pod/src/prune_hook.rs`: **削除** -- `crates/pod/src/pod.rs`: `PruneHook` 生成を `worker.set_prune_config` + - `worker.set_savings_estimator` に置き換え -- `crates/pod/src/lib.rs`: `prune_hook` モジュールの削除 - -### 得られるもの - -- llm-worker 側に prune 関連コードを集約できる -- `PreLlmRequest` hook から prune 固有の mutation が消える - （現状 hook に渡る `&mut Vec` はユーザー向け hook が自由に使えるが、 - Worker 内部の prune と同じ経路を共有するのは混乱の元） -- PruneHook の単体テスト不足問題（初回レビュー指摘2）も Worker のテストとして - 自然に書ける - -## レビュー状態 - -Reviewed — [prune-projection.review.md](prune-projection.review.md) -（追加作業「Worker への統合」は未着手） - -## 依存 - -- なし - -## ブロックする後続 - -- [compact-improvements.md](compact-improvements.md) — compact worker が content を参照する前提 diff --git a/tickets/prune-projection.review.md b/tickets/prune-projection.review.md deleted file mode 100644 index 2a5963c5..00000000 --- a/tickets/prune-projection.review.md +++ /dev/null @@ -1,122 +0,0 @@ -# prune-projection レビュー - -## 要件の充足 - -チケットが定義した3つの設計方針は全て達成されている: - -1. **判定結果を index 集合として保持** — `prunable_indices` がそのまま該当 -2. **PreLlmRequest hook でコンテキスト構築時に content を省く** — - `PruneHook::call` で `context` に対してのみ書き換え -3. **Worker の history を一切変更しない** — `worker.rs:701` の - `let mut request_context = self.history.clone()` により、hook に渡るのは - clone。`build_request` で使われた後に破棄され、`self.history` には戻らない - -## アーキテクチャ - -責務分離が綺麗に実装されている: - -- `llm-worker::prune` には pure な `prunable_indices` と `PruneConfig` のみ。 - mutation 系 API (`apply_prune`, `PruneResult`) は完全に削除 -- `PruneHook` 内に content ドロップの inline ループ。意図的にインライン化されており、 - 「射影は呼び出し側の責任」という設計が形になっている -- `PruneHook` の doc comment で `worker.rs:701` への参照により - 「なぜ mutate しても安全か」を明示 - -## 指摘と対処 - -### 1. clone 依存の脆さ（非ブロッカー、未対処） - -現在の安全性は `worker.rs:701` の clone に依存している。worker の実装変更で -clone が消えると設計が即座に壊れる。対策案: - -- worker.rs の clone 箇所に「この clone は射影 hook に依存されている」旨の - コメントを足す（軽い対処） -- `PreLlmRequest` hook の型を `RequestContext` のような専用型にして - 型レベルで分離（本格対処） - -将来的な事故防止の観点で気になるが、チケット範疇の外。 - -### 2. PruneHook の射影ロジックに単体テストが無い（非ブロッカー、未対処） - -`llm-worker::prune` の `apply_prune` テストを削除したのは正しいが、 -`PruneHook::call` の射影ロジックには単体テストがない。 -`PruneHook::call` を呼んで、context が変更され、かつ元 history が -変わらないことを検証する統合テストがあると安心。 - -## 判定（初回） - -承認。ただし初回レビューで「pure ロジックが `llm-worker::prune` にあるのに -適用は pod 側 Hook で行う」という責務の不整合が見つかり、追加作業 -「Worker への統合」をチケットに追記して再実装。 - ---- - -# 追加作業レビュー: Worker への統合 - -## 要件の充足 - -追加作業で定義した変更は全て実装されている: - -| 項目 | 実装 | -|---|---| -| `Worker::set_prune_config` | `worker.rs:317` | -| `Worker::set_savings_estimator` | `worker.rs:329` | -| `build_request` 直前での射影 | `worker.rs:733-758` | -| `SavingsEstimator` 型定義 | `llm-worker::prune::SavingsEstimator` | -| `pod::prune_hook` モジュール削除 | 削除済み | -| Pod 側は config と estimator を渡すだけ | `pod::prune::attach_prune` | - -Worker が prune の責任を持ち、pod は usage 履歴に依存する estimator -コールバックを注入するだけ、という責務分離はチケット通り。Locked 状態への -lock 時にも `prune_config` / `savings_estimator` が保持される点も対処済み -(`worker.rs:1214-1217, 1286-1289`)。 - -## 指摘と対処 - -### A. `attach_prune` がどこからも呼ばれていない（未対処、要判断） - -`Pod::attach_prune` は実装されているが、コードベース内で呼び出し箇所が無い。 -履歴を見ると、リファクタ前の `PruneHook` も一度も registration されていない -デッドコードだった (`be1119d` 以降 `PruneHook::new` を呼んだ箇所無し)。 -つまりこのリファクタは「デッドコードを別の形のデッドコードに置き換えた」状態。 - -チケットの「追加作業」範疇としては設計通り完結しているが、prune 機能が実際に -有効化されていないのは事実。Manifest や `Controller::spawn` のどこで -`attach_prune` を呼ぶかは別チケット扱いにするか、このチケット内で一緒に -対処するかを要判断。 - -### B. 閉包内でのロック取得（非ブロッカー、未対処） - -`attach_prune` の estimator は毎回 `usage.lock().expect(...).clone()` する。 -現在 `usage_history` を触る箇所は: - -- `Pod::persist_turn`（run 終了後、短時間） -- `Pod::compact`（同上） -- `Pod::usage_history()`（snapshot 取得、短い） -- estimator（request ごと、clone のみ） - -estimator 発火は worker.rs の `build_request` 直前、つまり Pod の `run()` が -待機中なので他のロック取得と並走しない。現時点では安全。将来 `usage_history` -を別スレッドから触るコードが増えた時の事故防止として、pod.rs の -`usage_history_handle` doc コメントに「Mutex は短時間 clone 専用」という -前提を明示すべき。 - -### C. Worker 内部のインラインループ（非ブロッカー、未対処） - -`worker.rs:733-758` に content 射影のインラインループが直書き。`prune` -モジュール側に `apply_projection(&mut Vec, &[usize])` のような -pure 関数を用意して Worker はそれを呼ぶだけにすれば、Worker のメインループが -短くなり、`prune` モジュール内でのテストも書きやすくなる。現状 Worker の -`run` ループが肥大化する方向に寄っている。 - -### D. 射影ロジックに単体テストが無い（初回指摘2から継続、未対処） - -Worker に統合したことで Worker のテストとして書きやすくなったはずだが、 -テストは追加されていない。指摘 C で `apply_projection` に切り出せば、 -その pure 関数単位でテストが書ける。 - -## 判定（追加作業） - -条件付き承認。指摘 A が機能有効化の観点で重要。意図通り（別チケットで -manifest 配線）なら承認、このチケット内で対処するなら未完了扱い。 -指摘 B/C/D はコード品質の改善で非ブロッカー。