From 46390a9006fa0ef89d8a98db9a9bec8167ea1e9e Mon Sep 17 00:00:00 2001 From: Hare Date: Sun, 3 May 2026 21:34:54 +0900 Subject: [PATCH] =?UTF-8?q?docs(tickets):=20session-todo=20=E3=83=AC?= =?UTF-8?q?=E3=83=93=E3=83=A5=E3=83=BC=E5=8F=8D=E6=98=A0=20(Approve)=20+?= =?UTF-8?q?=20reminder=20spec=20=E6=AE=B5=E9=9A=8E=E3=83=AC=E3=83=93?= =?UTF-8?q?=E3=83=A5=E3=83=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tickets/session-todo-reminder.md | 7 ++ tickets/session-todo-reminder.review.md | 41 ++++++++++++ tickets/session-todo.md | 6 ++ tickets/session-todo.review.md | 87 +++++++++++++++++++++++++ 4 files changed, 141 insertions(+) create mode 100644 tickets/session-todo-reminder.review.md create mode 100644 tickets/session-todo.review.md diff --git a/tickets/session-todo-reminder.md b/tickets/session-todo-reminder.md index d0a1261a..1847cb4f 100644 --- a/tickets/session-todo-reminder.md +++ b/tickets/session-todo-reminder.md @@ -58,3 +58,10 @@ Insomnia でも同方針を採り、active Task が残っているのに `TaskCr - 設計指針: `CLAUDE.md`（最小の構造化 / 概念の追加は不在が問題になってから） - 前提: `tickets/session-todo.md`（Tool 群と TaskStore） - 参考実装: Claude Code の `task_reminder`（`du_` / `cu_` 関数、閾値 `Z_8 = { TURNS_SINCE_WRITE: 10, TURNS_BETWEEN_REMINDERS: 10 }`） + +## Review + +- 状態: Approve (spec 段階) +- レビュー詳細: [./session-todo-reminder.review.md](./session-todo-reminder.review.md) +- 日付: 2026-05-03 +- 補足: 実装着手前に Non-blocking で挙げた 4 点 (TaskCreate/Update のカウンタリセット契機 / active 取得経路 / reminder 本文 fmt / resume 時のカウンタ扱い) を spec に追記してから実装するとブレが減る。実装完了時に完了条件を再確認。 diff --git a/tickets/session-todo-reminder.review.md b/tickets/session-todo-reminder.review.md new file mode 100644 index 00000000..15cd447c --- /dev/null +++ b/tickets/session-todo-reminder.review.md @@ -0,0 +1,41 @@ +# Review: セッション内 Task ツールの注意機構 + +対象: `tickets/session-todo-reminder.md` (`28fe1da` で新規作成)。実装は未着手 (本レビュー時点でコード変更なし、worktree 内 grep で `task_reminder` / `requests_since_last_task` 等いずれもヒットせず)。 + +本レビューは spec 単独レビュー (実装が始まった時点で完了条件を再確認)。 + +## 前提・要件の確認 (spec の妥当性) + +- 「`pre_llm_request` で `Vec` を受けて直近 user message に `` を append する」は、`crates/pod/src/ipc/interceptor.rs` の既存 `pre_llm_request` レーンに自然に乗る。Worker history を変更せず、リクエスト送信用 `Vec` のみ加工する方針は、既存の `pending_notifies.drain()` と同じパターンで先例あり (`interceptor.rs:147-159`)。spec として実装可能性は高い。 +- TaskStore からの active 件数取得には A の `TaskStore::list()` だけで足り、追加 API 不要。 +- カウンタ (`requests_since_last_task_management` / `requests_since_last_reminder`) を Pod の session-lifetime 状態として持つ方針は、既存 `tool_calls_this_turn: AtomicUsize` (`interceptor.rs`) と同じ素地に追加できる。pre_tool_call で `TaskCreate` / `TaskUpdate` を観測したら片方をリセット、`pre_llm_request` で発火条件を見て両方を増やす、という骨格になる想定。spec の粒度は問題ない。 +- メインスレッド限定 / sub-agent 除外: 主要 worker と spawn pod / compact worker を分離している現実装では「メインの `PodInterceptor` だけがカウンタを持つ」で達成できる。compact worker 側は別 Worker / 別 Interceptor。spec の前提は現実装と一致。 +- resume 時のカウンタ復元: 「履歴の逆走査で再計算 or 0 リセット」のどちらでも実害無しと spec 側で割り切られているのは妥当。0 リセットの方が大幅にシンプルで、初回ナッジが最大 N 遅れるだけ。実装時はこちらを推奨したい (spec 側は両許容で問題なし)。 +- 「揮発的 = 履歴を汚さない」「タグ形式 `...` で揃える」「2 件目の利用者が出た時に汎用化を検討」の 3 点を明文化していて、A と B の隙間に「汎用 reminder 機構」の中途半端な抽象が立たないようにしている。CLAUDE.md の「概念の追加は不在が問題になってから」と整合。 + +## アーキテクチャ・スコープ + +- B は A に対する後続チケットとして適切に分離されている。前提依存 (TaskStore + Task* tools) を `## 前提` に明記しており、A 完了後に着手するという順序付けも自然。 +- 範囲外 (inprogress 滞留 / 多重 inprogress / 注入機構汎用化 / Tool 戻り値の active 全件埋め込み) が列挙されており、本チケットで肥大化しないラインが引けている。 +- 「Pod 側に session-lifetime カウンタを増やす」程度の侵襲で、既存の `crates/pod/src/ipc/interceptor.rs` 内に追加実装が収まる規模感。クレート構成や層境界に新しい歪みは生じない見込み。 +- 「N=8 / M=8 暫定」と数値を明示しているのは判断しやすい。Claude Code が `Z_8 = { TURNS_SINCE_WRITE: 10, TURNS_BETWEEN_REMINDERS: 10 }` という参照値を持つことも書かれており、後で詰めやすい。 + +## 指摘事項 + +### Non-blocking / Follow-up (spec 段階の論点) + +- **「リクエスト = LLM への 1 回の推論呼び出し」のカウント単位が、現 `Interceptor` の `pre_llm_request` 1 呼び出しと一致するか確認が必要。** 現実装の `pre_llm_request` は context 構築直前に必ず通るレーンで、tool ループ内の続行 LLM 呼び出しでも毎回呼ばれる。実装時に「pre_llm_request 突入時刻 = LLM へのリクエスト 1 件」が成立しているかを軽く検証するテストを1本入れてほしい (tool ループ中の発火 cadence のため)。 +- **TaskCreate / TaskUpdate のカウンタリセット契機。** 現状 spec は「呼ばれていない」と書かれているのみ。実装ではどのフックで観測するかの選択 (`pre_tool_call` か `post_tool_call` か) を決めて spec に追記しておくと実装ブレが減る。`pre_tool_call` で名前判定 + リセット、で十分。 +- **active Task の取り出し経路。** spec 上は明示されていないが、Interceptor が TaskStore handle を保持する形になる (Pod 経由で渡す)。`PodInterceptor::new` 等のコンストラクタに `Option` を増やす想定で良いと思うが、実装時に `Tracker` の例 (`attach_tracker`) と同様、Pod から Interceptor への手渡しを統一してほしい。 +- **resume 時の発火タイミング。** 0 リセットを採るなら、resume 直後の最初のリクエストでは出ない。これは spec 上「最大 N 遅れるだけ」と許容済み。実装時は単にこの選択を README/コメントに残してほしい。 +- **`` の本文。** spec は「taskid / status / subject を簡潔に列挙」と書かれているが、A 側の `render_snapshot` を流用するか、reminder 専用に別関数を切るかが未決定。短さを優先するなら専用 fmt が望ましい (description を必ず落とすため)。実装方針を spec に1行追記しておくと安全。 +- **テスト範囲。** spec の完了条件にある「リクエスト回数閾値 / active 0件 / cooldown / サブエージェント除外」は単体テストでカバー可能。実装時はそれぞれ独立ケースで書いてほしい。 + +### Nits + +- 数値 N / M が「暫定 N=8」「暫定 M=8」と明記されているが、変更可能性についてのフォローアップ条項 (例: 運用後に閾値を見直す) があると親切。 +- Claude Code 側の参照値 `Z_8 = { TURNS_SINCE_WRITE: 10, TURNS_BETWEEN_REMINDERS: 10 }` は spec に書かれているので、Insomnia 側 8 を採る理由 (短めにして気付かせやすくする等) を一文添えると将来 tuning しやすい。 + +## 判断 + +**Approve (spec 段階)** — 前提・要件・範囲外の切り分けが妥当で、A の実装規約 (TaskStore handle、揮発的注入、タグ形式) に乗る形で実装可能。アーキテクチャ的歪みも生じない見込み。実装着手前に上記 Non-blocking 4 点 (カウンタリセット契機 / active 取得経路 / 本文 fmt / resume 時のリセット選択) を spec に1〜2行追記してから実装に入ると、実装中の判断ブレが減る。実装完了時に完了条件を再確認する必要がある (本レビューは spec 妥当性のみの判定)。 diff --git a/tickets/session-todo.md b/tickets/session-todo.md index 3dc27291..ee187cbf 100644 --- a/tickets/session-todo.md +++ b/tickets/session-todo.md @@ -103,3 +103,9 @@ - 参考実装: Claude Code の TodoWrite、OpenCode の todo tool - 関連: `crates/tools/src/tracker.rs`（session-lifetime 状態の前例）、`crates/pod/src/compact/worker.rs`（auto-injection レーン） - 後続: `tickets/session-todo-reminder.md`（注意機構） + +## Review + +- 状態: Approve (再レビュー済み、blocking 解消・non-blocking 対応反映) +- レビュー詳細: [./session-todo.review.md](./session-todo.review.md) +- 日付: 2026-05-03 diff --git a/tickets/session-todo.review.md b/tickets/session-todo.review.md new file mode 100644 index 00000000..9b447de4 --- /dev/null +++ b/tickets/session-todo.review.md @@ -0,0 +1,87 @@ +# Review: セッション内 Task ツール + +対象: `tickets/session-todo.md` (`28fe1da` で再分割) / 実装 `6f2aca8`。 + +## 前提・要件の確認 + +- TaskCreate / TaskList / TaskGet / TaskUpdate を builtin tool として登録: 充足。`crates/tools/src/task.rs:507` の `task_tools(store)` を `crates/tools/src/lib.rs:55` の `builtin_tools` が拡張し、Controller 経由で Worker に登録される (`crates/pod/src/controller.rs:268`)。 +- TaskCreate の自動採番 + 初期 status pending: 充足 (`crates/tools/src/task.rs:71`)。 +- TaskUpdate の最低1フィールド + 既存チェック + 物理削除なし (`status=deleted`): 充足 (`crates/tools/src/task.rs:108`、テスト `task_update_validates_existing_and_at_least_one_field`)。 +- TaskList が completed / deleted を含めて全件返す: 充足 (`crates/tools/src/task.rs:84`)。 +- TaskGet が `taskid` 不在でエラー: 充足 (`crates/tools/src/task.rs:304`)。 +- Resume 時の履歴 replay で TaskCreate / TaskUpdate を順に再適用: 実装あり (`replay_history`、`Pod::from_state` で `TaskStore::from_history(&state.history)`)。**ただし compact 後の resume で重複が発生する** (後述「Blocking」)。 +- `tool_call.arguments` の schema 乖離は無視: 充足 (`from_str` 失敗時 `_` でスキップ)。 +- compact 起動時、TaskStore 全スナップショットを compact worker context に渡す: 充足 (`build_summary_input` の `task_snapshot` 引数、`crates/pod/src/pod.rs:1278`)。 +- compact worker に Task 編集権限を与えない: 充足 (compact 用 `summary_worker.register_tool` は read / mark_read_required / add_reference / write_summary のみ。Task* tool は注入されていない)。 +- compact 後の新セッションで TaskStore handle を維持し全件保持: 充足 (`self.task_store` は compact 中に置換されない。Worker の history のみ入れ替え)。 +- compact 後の新セッション冒頭に snapshot system message + TaskList 発火: 充足 (`crates/pod/src/pod.rs:1409-1419`)。 +- 専用 LogEntry / Persistence 型を追加しない: 充足 (`replay_history` と inline snapshot text に閉じている)。 +- 単体テスト: 4 ツール基本 / バリデーション / replay / compact snapshot replay の4本。**retained tail に Task* call が残るシナリオの replay テストが欠落** (後述)。 + +## アーキテクチャ・スコープ + +- `tools` クレートの session-lifetime 状態として TaskStore を置き、Pod が handle を所有して `attach_task_store` で受け渡す方針は `Tracker` の前例と整合。レイヤー越境なし。 +- 4 ツールを `crates/tools/src/task.rs` の単一モジュールにまとめている点は、現時点の規模 (~600 行) と将来の機能追加余地を考えると許容範囲。`pod.rs` への impl 流入もない。 +- ツール本体は `tools` 層、配線と compact 連携は `pod` 層、と責務分離が保たれている。 +- TaskStore は `Arc>` の thin wrapper で、永続化を tool_call replay に寄せる方針 (専用レーン無し) もチケットの要件どおり。 +- 命名: `TaskStore` / `TaskEntry` / `TaskStatus` / `TaskSnapshot` の対称性も良。`insomnia-` prefix は無し (規約準拠)。 +- `task_store: tools::TaskStore` を pub field 露出ではなく `attach_task_store` / `task_store()` の getter で扱っており、これも `Tracker` の前例と整合。 +- compact 時の "synthetic tool_call" (`compact-tasklist` の TaskList call/result) は新規パターン。Pod 配下で Worker 履歴に注入される唯一のラインであり、再現性とプロバイダ受理性が運用上の検証対象になる (フォローアップ参照)。 + +## 指摘事項 + +### Blocking + +- **compact 跨ぎの resume で TaskStore が重複/二重適用される。** `crates/pod/src/pod.rs:1398-1423` で構築する post-compact history は `[summary, ...auto-read, snapshot system message, TaskList synthetic call/result, references, ...retained_items]` の順だが、`retained_items` は compact 直前まで生きていた末尾スライスをそのまま含むため、retained 内に `TaskCreate` / `TaskUpdate` が含まれた場合、resume 時の `TaskStore::from_history` が: + 1. 先頭付近の `[Session TaskStore snapshot]` を読み `replace_with` で全件復元 + 2. 後続の retained `TaskCreate` を再適用 → 同内容の重複 entry が新規 `taskid` で生える + 3. retained `TaskUpdate` も再適用 (こちらは冪等で害は薄いが意味は揺れる) + + になる。compact は token threshold 到達で起きるので、直近に Task* を呼んだケース (= 実際の主用途) で確実に発火する。 + + 既存テスト `replay_history_uses_compact_snapshot_and_continues_updates` (`crates/tools/src/task.rs:592`) は snapshot を history[0] に置き、後続を「compact 後の新規操作」として書いているため、本ケース (snapshot より後の retained_items に旧 Task* call が残る) を踏んでいない。 + + 対策案: + - (A) `pod.rs:1423` の `new_history.extend(retained_items)` の前に retained_items から `Item::ToolCall { name == "TaskCreate" | "TaskUpdate" .. }` と対応する `ToolResult` を除去する。snapshot + TaskList 再発火がポストコンパクトの唯一の真実になる + - (B) `replay_history` 側に anchor を入れ、snapshot を見たら**それ以前の** Task* call を捨てる + snapshot 以降は通常 replay。retained_items が snapshot より後ろにある現レイアウトでは効かないので、history 上の snapshot の位置を末尾 (retained_items の後) に移すなどレイアウト変更が必要 + - (C) compact 後 history 構築時に、snapshot の代わりに「TaskStore を空にしてから個別 TaskCreate で再現する」方針に切り替える (= snapshot の役割を tool_call 列で担う)。ただし冗長なので非推奨 + + 推奨は (A)。あわせて回帰テストとして「retained_items に旧 TaskCreate を含む post-compact history を `from_history` に通し、duplicate が出ないこと」のケースを追加してほしい。 + +### Non-blocking / Follow-up + +- **snapshot text のラウンドトリップが lossy。** `render_snapshot` は `subject` / `description` を1行 inline で書き出すので、本文に改行が含まれると `parse_rendered_snapshot` が後続行を読み落とし、subject/description の途中で切れる (`crates/tools/src/task.rs:377-433`)。`description` は LLM が複数行で書く可能性が現実的なので、snapshot レンダリングを JSON ブロック (例: コードフェンスで `{"tasks":[...]}`) に切り替えるか、`description` に YAML literal block (`|`) を使うなどでロスレス化したい。replay の信頼性に直結する。 +- **compact 由来の synthetic ToolCall の provider 受理性。** `Item::tool_call("compact-tasklist", "TaskList", "{}")` + 対応する `ToolResult` が、対応する assistant turn なしに history 中に挿入される (`crates/pod/src/pod.rs:1414-1419`)。Anthropic / OpenAI / Codex 各 client がこの形を受理して次リクエストを通すか、E2E で1度通しておきたい。フォーマット・ID プレフィックス的に拒否されるリスクは低いが、未検証ライン。 +- **`PodInterceptor::pre_llm_request` の `&& !state.just_compacted()` 追加 (`crates/pod/src/ipc/interceptor.rs:130`) はチケット範囲外。** 内容自体は `try_post_run_compact` (`pod.rs:1079`) と同じガードを request-side にも回す mirror で、compact 直後の閾値再ヒットによる thrash 防止として妥当。とはいえ「session-todo」のスコープではないので、本来は `compact-thrash-fix` のような別チケットに切り出すか、ticket 本文に「副次修正」として明記するのが望ましい。今回はそのまま採用するなら commit message と `## 範囲外` の補記で痕跡を残してもらえると後追いが楽。 +- snapshot system message に「This is the complete session task list ...」と英文で説明が入っているが、TaskList 用の synthetic tool_call の summary には説明なし。LLM 体験としては snapshot system message → TaskList result の二段で同じ情報が出るので、片方に寄せて簡潔化しても良い (任意)。 + +### Nits + +- `crates/pod/src/pod.rs:1398` のコメント: `[summary, ...auto-read, task snapshot, TaskList result, references, ...retained]` だが実際は `task snapshot` の後ろに `TaskList tool_call` と `tool_result` の **2 件** が入る。3 件であることをコメントに反映。 +- `task.rs:79` `inner.next_taskid.saturating_add(1)`: u64 で saturating は事実上不要だが、害もない。 +- `lib.rs` の `pub use task::{TaskEntry, TaskSnapshot, TaskStatus, TaskStore, task_tools};` で `task::snapshot_overview` だけ `pod.rs` から `tools::task::snapshot_overview(...)` のフルパスで参照されている。`tools::snapshot_overview` 等で同レベル re-export しておくと API surface が揃う (任意)。 + +## 判断 + +**Request changes** — Resume 後の TaskStore 重複は要件「セッションを resume すると、履歴中の TaskCreate / TaskUpdate replay により TaskStore が復元される」「compact を跨いでも、TaskStore 全件が維持され」の両方に直接抵触する正解性 issue。Blocking 1点 (retained_items 二重適用) を直したうえで回帰テストを追加して再提出してほしい。アーキテクチャ・スコープ・残りの要件カバーは概ね健全で、`簡素な` 設計を保てている。 + +## 再レビュー (commit `420f74e` まで反映) + +判断: **Approve**。 + +### 対応状況 + +- **Blocking (retained 二重適用)**: 解消。`ceafff9` で post-compact history のレイアウトを `[summary, auto-read, references, ...retained, snapshot, TaskList synthetic]` に変更。snapshot を末尾に置き `replace_with` が常に最後勝ちにする設計に切り替えた。回帰テスト `trailing_snapshot_supersedes_pre_compact_taskcreates_in_retained` で「retained に pre-compact TaskCreate × 2 + TaskUpdate × 2 → snapshot → post-compact TaskCreate × 1」のシナリオが正しく 3 件で収束することを確認。 +- **Non-blocking #1 (snapshot text の lossy round-trip)**: 解消。`420f74e` で `render_snapshot` を「overview 一行 + ` ```json` フェンス内 JSON」形式に変更。`parse_compact_snapshot_text` も JSON フェンスから serde で復元する形に再実装。`snapshot_round_trips_multiline_subject_and_description` テストで改行入り subject/description + 紛らわしい `status:` プレフィックス + バッククォートが lossless で復元することを確認。`HashMap`-based 行パーサと `parse_rendered_snapshot` / `task_from_fields` ヘルパは削除。 +- **Non-blocking #2 (synthetic compact-tasklist の provider 受理性)**: 構造テストで部分対応。`synthetic_compact_tasklist_pair_is_well_formed` テストで「snapshot system message → TaskList tool_call → ToolResult」3 連の構造契約 (matched call_id、tool name、content recoverability) を確認。実プロセス E2E はプロジェクト全体で未設計 (CLAUDE.md 「E2E は未設計」) のためここでは構造的健全性までしか確認できない。実 provider 通過は将来の E2E 設計時に再確認する follow-up。 +- **Non-blocking #3 (`interceptor.rs:130` 範囲外修正)**: 範囲外注記として残す方針で確定。`!state.just_compacted()` ガードは `try_post_run_compact` の既存ガードを request-side に mirror する妥当な小修正で、本来は別チケットだが現規模では note 扱いとする (reviewer 提示の 2択のうち「範囲外注記」を採用)。今後 compact thrash 周りで関連修正が必要になった場合に別チケットへ切り出すフォローアップ余地は残る。 + +### Nit (`pod.rs:1398` コメントの 3 件挿入の不一致) + +レイアウト変更に伴い同じく更新済み (`ceafff9`)。新コメント: `[summary, ...auto-read, references, ...retained, task snapshot, TaskList synthetic call/result]` + 末尾配置の理由を明記。 + +### 残る follow-up + +- 改行入り subject/description のロスレス化は完了したが、TaskList が tool result として LLM に返す JSON フェンスのトークン効率は将来の課題 (大量 task 時) +- synthetic `compact-tasklist` の provider 受理性 E2E (上述、プロジェクト E2E 設計の中で扱う) +- `interceptor.rs` の compact thrash guard は compact 周辺の他 issue が表面化した際に専用チケット化を検討