prune-projection完了
This commit is contained in:
parent
2e004161e4
commit
7ec6e88605
1
TODO.md
1
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)
|
||||
|
|
|
|||
|
|
@ -1,91 +0,0 @@
|
|||
# Prune をコンテキスト射影に変更
|
||||
|
||||
## 背景
|
||||
|
||||
現状の `apply_prune` は Worker が保持する history の `Item::ToolResult { content }`
|
||||
を直接 `None` に書き換えている。PreLlmRequest hook の `context: &mut Vec<Item>` は
|
||||
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<dyn Fn(&[Item], Range<usize>) -> 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<Item>` はユーザー向け 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 を参照する前提
|
||||
|
|
@ -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<Item>, &[usize])` のような
|
||||
pure 関数を用意して Worker はそれを呼ぶだけにすれば、Worker のメインループが
|
||||
短くなり、`prune` モジュール内でのテストも書きやすくなる。現状 Worker の
|
||||
`run` ループが肥大化する方向に寄っている。
|
||||
|
||||
### D. 射影ロジックに単体テストが無い（初回指摘2から継続、未対処）
|
||||
|
||||
Worker に統合したことで Worker のテストとして書きやすくなったはずだが、
|
||||
テストは追加されていない。指摘 C で `apply_projection` に切り出せば、
|
||||
その pure 関数単位でテストが書ける。
|
||||
|
||||
## 判定（追加作業）
|
||||
|
||||
条件付き承認。指摘 A が機能有効化の観点で重要。意図通り（別チケットで
|
||||
manifest 配線）なら承認、このチケット内で対処するなら未完了扱い。
|
||||
指摘 B/C/D はコード品質の改善で非ブロッカー。
|
||||
Loading…
Reference in New Issue
Block a user