docs(tickets): notify-history-persist 完了 (消し忘れ片付け)
This commit is contained in:
parent
698175e60c
commit
8d9910fd20
|
|
@ -1,83 +0,0 @@
|
|||
# 注入される system message をワーカー履歴に永続化する
|
||||
|
||||
## 背景
|
||||
|
||||
現状、`Method::Notify` および `Method::PodEvent`（`TurnEnded` / `Errored` / `ShutDown` / `ScopeSubDelegated`）は、親 Pod 側で `NotifyBuffer` に積まれ、`PodInterceptor::pre_llm_request` で **その1リクエスト限り**のsystem messageとして注入される（`crates/pod/src/ipc/interceptor.rs:147-159`）。
|
||||
|
||||
しかし `Worker::run` は `let mut request_context = self.history.clone();` してから interceptor を呼ぶ（`crates/llm-worker/src/worker.rs:862, 913`）ため、interceptor が push した notification は clone 側にしか乗らない。一方、その後の LLM 応答（`assistant_items`）は `crates/llm-worker/src/worker.rs:946` で **本体の `self.history`** に extend される。
|
||||
|
||||
結果、履歴上は「文脈ゼロから Pod がいきなり `ReadPodOutput` を呼んだ」「何の前触れもなく `child Pod が落ちた件について調べる』と発言した」などの状態になる。次のリクエスト時点で既に LLM 自身の自己一貫性が壊れており、コンパクション・再起動を待たずして破綻している。
|
||||
|
||||
同じ問題は `tickets/session-todo-reminder.md` で予定している `<system-reminder>` 注入にも存在する。当初「reminder は同じ内容を繰り返し注入する可能性があるから履歴を汚さない」という方針を取っていたが、これは category error だった:
|
||||
|
||||
- **キャッシュ破壊**: 揮発で last user message を mutate する設計だと、worker.history 側は元のままなので、毎回 reminder の有無/内容差分で「実際に LLM へ送る user message の content」が変動する。Anthropic prompt cache は anchor までしか効かないため、anchor 直後の生成が毎回 cache miss になる
|
||||
- **LLM の自己一貫性**: turn N で reminder を見て `TaskUpdate` を叩いた → turn N+1 では reminder が消えて、自分の `TaskUpdate` 呼び出しだけ残る。Notify と全く同じ因果切断
|
||||
- **resume 時の不整合**: ロードした history からは reminder が完全消失している状態で再開する
|
||||
- **「繰り返し注入で履歴肥大」の前提も弱い**: cooldown 設計上 reminder は idle 期間に1回 + 反応で counter リセット。連発は元々しない。仮に複数回出ても、それぞれが「その時点での active Task の snapshot」として履歴に並ぶのは因果として正しい
|
||||
|
||||
つまり「LLM に投げた system message は、その時点で history に commit する」が原則で、Notify / PodEvent / `<system-reminder>` を一律にこの原則に揃える。
|
||||
|
||||
## 方針
|
||||
|
||||
- LLM リクエスト直前に注入される system message は、`request_context: &mut Vec<Item>` の clone 側ではなく **worker 本体の `history` 側** に append する
|
||||
- `NotifyBuffer` は **「次のリクエスト直前で `worker.history` に append するキュー」** として再定義する
|
||||
- 永続化（`history.json`）は worker.history 経由で自動的についてくる（`PodSharedState::history_json` → `RuntimeDir::write_history`）
|
||||
- 対象は現状の `Method::Notify` / `Method::PodEvent` に加え、session-todo-reminder で予定している `<system-reminder>` 注入も含む（後者は前者と同じ NotifyBuffer に乗せるか別キューを立てるかは実装裁量。重要なのは「history に commit される」点）
|
||||
- `notify_wrapper` の文言（`[Notification] ... not a blocking request`）はそのまま履歴に残してよい。後から見ても「これは ambient 通知だった」と分かる方が望ましい
|
||||
- `<system-reminder>` も同様、タグ込みのまま history に残す（タグ形式 `<system-reminder>...</system-reminder>` の規約自体は維持）
|
||||
|
||||
## 要件
|
||||
|
||||
### Notify / PodEvent 経路の挙動変更
|
||||
|
||||
- `NotifyBuffer::drain` 由来の Item は `request_context` ではなく `worker.history` に append される
|
||||
- append は **次の LLM リクエスト直前** に1回だけ起きる（複数 notify が貯まっていれば順序を保って複数 Item として並ぶ）
|
||||
- append 後、`history.json` への永続化が通常の history mutation と同じパスで起きる
|
||||
- 永続化された Item は次回 resume 時にそのまま履歴の一部として読み戻される
|
||||
|
||||
### `<system-reminder>` 注入経路（session-todo-reminder の前提変更）
|
||||
|
||||
- `<system-reminder>` ブロックを「直近 user message を mutate して append」する設計を撤回し、**新規 system message Item として `worker.history` に append** する形に変更する
|
||||
- ライフサイクルは Notify / PodEvent と同じ: 注入条件を満たした時点で history に commit、`history.json` に永続化、resume 後も読み戻される
|
||||
- session-todo-reminder.md 側の「履歴を汚さない」「`get_history` / セッションログには現れない」「last user message を mutate」記述は本ticketの方針で上書きする
|
||||
|
||||
### 注入レーンの統一
|
||||
|
||||
- 「LLM リクエスト直前に注入される system message」は一律 history レーンに乗せる、と `crates/pod/src/ipc/notify_buffer.rs` のモジュールdocに明記する
|
||||
- 「揮発（history を汚さない）レーン」の概念は廃する。将来 reminder 系を追加する際も同じ原則に従う
|
||||
- 命名・配置を見直す必要があれば実装内で判断してよい（例: `NotifyBuffer` を `PendingSystemMessages` 等に改称、reminder 用の別キューを作る等）。本ticketは挙動の正しさが最優先で、抽象の作り方は実装者裁量
|
||||
|
||||
### 既存テスト・ドキュメントの更新
|
||||
|
||||
- `crates/pod/src/ipc/interceptor.rs` の `pre_llm_request_drains_pending_notifies_into_context` 系テストは、`request_context` ではなく `worker.history` への反映を検証する形に書き換える
|
||||
- `crates/pod/tests/pod_events_test.rs` で `PodEvent` 受信後に history に対応 Item が現れることを E2E に近い粒度で確認するケースを追加する
|
||||
- 既存の「揮発レーン」前提のコメント（`crates/pod/src/ipc/notify_buffer.rs:5` の `(never into the Worker's persistent history)` 等）を新方針に合わせて書き換える
|
||||
- `tickets/session-todo-reminder.md` の方針記述を本ticketの完了に合わせて更新する（または本ticket完了時点で先行修正してよい）
|
||||
- `TODO.md` 末尾の「タグ形式と『履歴を汚さない』原則は session-todo で先行確立」記述から後者を撤回する
|
||||
|
||||
## 完了条件
|
||||
|
||||
- 親 Pod が `Method::Notify` または `Method::PodEvent` を受信すると、その後の最初の LLM リクエスト直前に対応 system message が **`worker.history` に append** され、リクエストにも含まれる
|
||||
- 同じ Item が `history.json` に書かれており、`Pod::resume` 後に履歴の一部として読み戻される
|
||||
- LLM が notification に反応して取った行動（tool call / 応答）と、そのトリガーとなった notification Item が、履歴上で因果順に並んでいる
|
||||
- session-todo-reminder で導入される `<system-reminder>` 注入も同じく history append として実装される（または、実装順次第で本ticketは Notify / PodEvent 側だけ完了させ、session-todo-reminder 実装時にこの原則に従う形でもよい。後者の場合は session-todo-reminder.md 側の方針記述を本ticket完了時に更新済みであることが必須）
|
||||
- 単体テストで上記が確認できる
|
||||
|
||||
## 範囲外
|
||||
|
||||
- `notify_wrapper` の文言・phrasing の見直し
|
||||
- TUI 側の Notify / PodEvent 表示（`Event::PodEvent` 経路は既存通り）
|
||||
- compaction 時の notify Item の扱い（通常 Item と同じく compaction 対象になればよい。特別扱いは不要）
|
||||
- `<system-reminder>` 注入機構の汎用化（`TODO.md` の既存項目。本ticketは個別実装の方針統一だけ扱う）
|
||||
|
||||
## 参照
|
||||
|
||||
- 設計指針: `CLAUDE.md`
|
||||
- 関連: `crates/pod/src/ipc/notify_buffer.rs`、`crates/pod/src/ipc/interceptor.rs`、`crates/pod/src/ipc/event.rs`、`crates/pod/src/controller.rs`（受信ハンドリング）、`crates/llm-worker/src/worker.rs:862, 913, 946`（clone する側）
|
||||
- 方針反転対象: `tickets/session-todo-reminder.md`（「履歴を汚さない」前提を本ticketで撤回）
|
||||
|
||||
## Review
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./notify-history-persist.review.md](./notify-history-persist.review.md)
|
||||
- 対象コミット: `e804577 feat: notify-history-persist実装`
|
||||
- 日付: 2026-05-03
|
||||
|
|
@ -1,47 +0,0 @@
|
|||
# Review: 注入される system message をワーカー履歴に永続化する
|
||||
|
||||
対象コミット: `e804577 feat: notify-history-persist実装`
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
### Notify / PodEvent 経路の挙動変更
|
||||
- **`NotifyBuffer::drain` 由来の Item を `worker.history` に append**: 達成。`crates/llm-worker/src/worker.rs:859-867` で turn loop の先頭、per-request clone の前に `interceptor.pending_history_appends().await` を `self.history.extend(...)` で本体側に commit する。`crates/pod/src/ipc/interceptor.rs:127-145` で旧 `pre_llm_request` の注入ロジックを新メソッドに移送。
|
||||
- **次の LLM リクエスト直前に1回だけ、複数 notify は順序保持で並ぶ**: 達成。`drain()` は破壊的、ループ各 iteration で1回呼ばれる。`pending_history_appends_drains_buffer_into_items` テストで複数 entry が順序保持で返ること、再呼出で空が返ることを検証。
|
||||
- **`history.json` への永続化**: 達成。`worker.history` への通常の mutation と同じ経路（既存の `PodSharedState::history_json` → `RuntimeDir::write_history`）に乗る。実装上の追加配線は不要で、設計意図通り。
|
||||
- **resume 時に読み戻される**: 達成（同上、既存パスを通る）。
|
||||
|
||||
### `<system-reminder>` 注入経路（session-todo-reminder の前提変更）
|
||||
- 本コミットでは reminder 実装そのものは扱わず、`tickets/session-todo.md` の方針記述のみ書き換え。`pending_history_appends` のドキュメント (`crates/llm-worker/src/interceptor.rs:121-148`) が新規 system message を history に commit する責務であることを明示し、reminder もこのレーンに乗ることが構造上保証される。チケットが許容する後者パターン（session-todo-reminder 実装時に同原則に従う）を取っており、整合済み。
|
||||
|
||||
### 注入レーンの統一
|
||||
- **`notify_buffer.rs` モジュール doc**: 達成。`crates/pod/src/ipc/notify_buffer.rs:1-19` で「single lane for system messages produced by Pod state」「no transient, history-skipping lane」を明記し、`tickets/notify-history-persist.md` と `AGENTS.md` を参照。
|
||||
- **trait レベルの規約**: `crates/llm-worker/src/interceptor.rs:121-148` に `pending_history_appends` と `pre_llm_request` の役割を doc で対比。`pre_llm_request` を「決定的・履歴非依存変換」のみに使い、外部 input は前者に乗せる旨を明記。CLAUDE.md / AGENTS.md の「LLM コンテキストの加工原則」の体現として適切。
|
||||
|
||||
### 既存テスト・ドキュメント更新
|
||||
- `pre_llm_request_drains_pending_notifies_into_context` → `pending_history_appends_drains_buffer_into_items` にリネームし、新レーン側の挙動検証に置換。
|
||||
- `pre_llm_request_skips_notification_injection_when_yielding` → `pre_llm_request_does_not_touch_pending_notifies` に変更。新方針上、yield 経路で buffer を保持する責務は不要（history に既に commit 済みであれば resume 後に再取得不要）なので、合理的な置換。
|
||||
- `controller_test.rs` の `notify_while_idle...` および `pod_event_turn_ended_while_idle...` に `handle.shared_state.history()` への assertion を追加し、request 側だけでなく history 側にも反映されることを検証。
|
||||
- `tickets/session-todo.md` / `TODO.md` 末尾の「履歴を汚さない」記述を撤回・上書き済み。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- **責務分離**: `Interceptor` trait に default 実装付きの新メソッドを1個追加し、Worker が turn loop で1回呼ぶだけ。llm-worker 層は「外部 input の中身」を知らず、低レベル基盤として hook されるだけ、という layer 区分を維持している。
|
||||
- **抽象の規模**: 新規概念は `pending_history_appends` 1個のみ。NotifyBuffer の改名や別キュー新設には踏み込まず、ticket の「実装裁量」のうち最小コストの選択（既存 buffer をそのまま流用、レーンの解釈を doc で更新）を取った。歪みなし。
|
||||
- **挙動の対称性**: 旧来 yield 時に buffer を保持していた設計が消えるが、新原則では「外部 input は受信時点で history 化されるべき」なので、turn loop 先頭での無条件 append は正しい。compaction 後 resume では buffer は既に空・履歴に entry あり、という状態に揃うのが意図通り。
|
||||
- **LLM コンテキストの加工原則との整合**: 本ticketは原則そのものの適用であり、doc 側でも `pre_llm_request` と `pending_history_appends` の責務境界を明示している。原則に正面から沿った変更で、コードベースを歪めるどころか、これまでの破綻を正している。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
- なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
- **ticket が名指しした test ファイル位置との乖離**: ticket 要件に「`crates/pod/tests/pod_events_test.rs` で `PodEvent` 受信後に history に対応 Item が現れることを E2E に近い粒度で確認するケースを追加する」と明記されているが、本コミットは既存 `controller_test.rs::pod_event_turn_ended_while_idle_auto_starts_turn_and_injects_system_message` に history assertion を追加する形で代替している。機能カバレッジは同等以上（既存テストを重複させない方が健全）だが、文字通りの要件には合わない。今後の維持で問題は出ない見込みだが、必要なら ticket 側を「カバレッジは controller_test.rs 側で確保済み」と注記しておくと将来の読み手が混乱しない。
|
||||
- **compaction yield 経路の挙動が暗黙化**: 旧 `pre_llm_request_skips_notification_injection_when_yielding` は「yield 中は buffer に積み残す」を保証していた。新設計では「turn loop 冒頭で必ず history に commit、yield しても history は残る」だが、これを直接検証するテスト（compaction 発火 → resume 後に history へ notify が残ること）は追加されていない。既存 compaction テストが history 側で間接的に保護してくれる範囲だが、回帰検出力としては薄い。session-todo-reminder で reminder を載せる際に同種の検証を入れる前提なら follow-up で十分。
|
||||
|
||||
### Nits
|
||||
- `crates/pod/src/ipc/interceptor.rs:14-22` の use 順序: `tracing::warn`（line 20）が `llm_worker::interceptor::...`（line 16-19）と `llm_worker::tool::ToolOutput`（line 21）の間に入り、`llm_worker::` グループが分断されている。`tracing::warn` を `tracing::info` の隣（line 22）に置けば従来のグルーピングが保てる。`cargo fmt` がこれを揃えているなら無視可。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve（完了可）** — 要件は実質すべて満たされており、CLAUDE.md / AGENTS.md の「LLM コンテキストの加工原則」を体現する模範的な変更。ticket が名指しした test ファイル位置との微妙な不一致は機能カバレッジ等価のため非ブロッキング。
|
||||
Loading…
Reference in New Issue
Block a user