Hare/yoi
1
1
Fork 0
Code Actions Packages Releases Activity
842e7a3c58
yoi/tickets/session-grouping-introduce.review.md
Hare 842e7a3c58 update: session-grouping review follow-up 
- PickerOutcome::Picked から未使用の session_id を除去（pod-cli が lookup_session_of で再解決）
- picker preview が singular AssistantItem も拾うように
- fs_store layout doc に migration（後方互換なし、旧 flat sessions は破棄）を明記
- TaskStore は Session-lifetime、ScopedFs/Tracker は Pod-process lifetime と用語整理
- Pod::session_id / from_manifest_spawned のコメント補強
2026-05-20 06:29:37 +09:00

107 lines
16 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Review: session-store: Session（Segment 群の grouping）導入
対象コミット: `a6cc05f feat: Session（Segment 群の grouping）を導入`
ベース: `develop` 上の `e4cda5d Merge: segment-rename`
## 前提・要件の確認
- **新 `SessionId` 型を `session-store` に追加。Segment は `parent_session_id` を持つ**:
達成。`SessionId` 型 alias と `new_session_id()` が
`crates/session-store/src/lib.rs:65-73`。
`LogEntry::SegmentStart { session_id, .. }` が
`crates/session-store/src/segment_log.rs:43`（serde 上は必須フィールド、`#[serde(default, skip_serializing_if = ..)]` の付かない裸の `session_id`）。`RestoredState.session_id: Option<SessionId>` は
`segment_log.rs:202` に追加され、`collect_state` で `SegmentStart` から拾われる（`segment_log.rs:258`）。
- **Segment 生成パスの session_id 決定（new / compaction / fork）**:
- new session: `create_segment` (`segment.rs:30`)、`Pod::new` (`pod.rs:495`)、`Pod::from_manifest` (`pod.rs:2786`)、`from_manifest_spawned` (`pod.rs:2867`) が `new_session_id()` を発行。`session_store::fork` (`segment.rs:406`) も同じく新発行。
- compaction: `create_compacted_segment` (`segment.rs:75`) と `Pod::compact` (`pod.rs:2188`) が `source_session_id` を継承。
- in-Session fork: `fork_at` (`segment.rs:454`) と `ensure_segment_head` の auto-fork (`pod.rs:1689`) が `source_session_id` を継承。
- `Pod::from_manifest_spawned` は spawn 元と独立した新 Session を発行 → child Pod は親 Pod と別 conversation 扱い。設計上の明確化は欲しいが、本 ticket の要件範囲では妥当。
- **`SessionOrigin` の整理（`compacted_from` / `forked_from`、同 Session 内 segment への参照）**:
実装は `SegmentOrigin { segment_id, at_turn_index }` を保持し（`segment_log.rs:181`）、両 origin は `LogEntry::SegmentStart` の `forked_from` / `compacted_from` フィールドに入る（`segment_log.rs:50, 54`）。同 Session 内参照は **コード経路で保証**（`create_compacted_segment` / `fork_at` の構築時 `source_session_id` 引数を経由する形）であって、**型レベルでは保証されていない**。ticket 要件「型レベルで保証」とは厳密には乖離する（非ブロッキング、後述）。
- **restore API を `(SessionId, SegmentId)` に**:
達成。`Store::*` 全 method が `(session_id, segment_id)` を取る (`store.rs:46-99`)。`restore` (`segment.rs:97`) も同様。`SegmentId` のみを取る legacy shim は `restore_by_segment` (`segment.rs:111`) として一段噛ませてあり、内部で `Store::lookup_session_of` を呼ぶ。pod-cli `--session` (`pod/src/main.rs:188`) と TUI の `load_resume_scope` (`tui/src/spawn.rs:332`) がこの shim を経由している。
- **FsStore layout に Session 単位の index**:
達成。`<root>/<session_id>/<segment_id>.jsonl` で Session ディレクトリ配下に Segment file を並べる (`fs_store.rs:5-8`)。`list_segments(session_id)` (`fs_store.rs:123`) は当該 Session ディレクトリ scan で済むので `WHERE session_id = ?` 相当。
- **Session 内の leaf Segment 列挙 API**:
`Store::list_segments(session_id)` (`store.rs:63`) が UUID v7 の lexicographic 順（時系列順）で最新→古い順に返す (`fs_store.rs:142`)。leaf は先頭 1 件で取れる（picker で実際にそうしている: `picker.rs:96-101`）。専用の `latest_segment_of` 等は無いが、`list_segments(sid).first()` で十分な最小実装。
- **migration 戦略**:
ticket 要件は「既存 sessions ディレクトリは破棄して良いかどうかをここで明示」。実装は新 layout (`<root>/<session_id>/<segment_id>.jsonl`) に切り替え、旧 flat `<root>/<segment_id>.jsonl` は `list_sessions` (`fs_store.rs:103-121`) の dir-type filter で自動的に skip されて見えなくなる。即ち「実質破棄」されている。**コード上の comment ではこの方針が明示されておらず**（`fs_store.rs:1-8` の Layout doc には触れていない）、ticket 完了条件「明示」が docs/code どちらにも乗っていない点で部分達成。
- **`cargo check --workspace` + 全テスト pass**:
確認。`cargo check --workspace` は `Finished dev` まで通る。`cargo test --workspace -- --skip double_run_returns_error` も全 suite green（warning 6 件、後述 nits）。`double_run_returns_error` は KNOWN_ISSUES.md 既登録の flake。
## アーキテクチャ・スコープ
- **層境界の維持**: 新 `SessionId` 型は session-store の最下層に置かれ、pod / pod-cli / TUI / pod-registry / session-metrics が同じ型を利用する形。`llm-worker` には Session 概念が滲んでいない（cache-key として segment_id を渡すのみ）。低レベル基盤としての session-store の役割を保っている。
- **`SegmentLocation` の atomic swap 設計**: `(session_id, segment_id)` を `ArcSwap<SegmentLocation>` で一括 swap (`pod.rs:48-94`)。fork / compaction で session_id と segment_id を**同時に**更新する必要があるため、別々の atomic 変数に分けると中間状態を観測される。1 struct で wrap した設計は妥当。`set_segment_id` の置き換え (`set_location`) もシグネチャ上 (session_id, segment_id) ペアを取るので安全。
- **`SegmentOrigin` の同 Session 性**: 上述の通り型レベルでは保証されない（`SegmentOrigin` 単体は `(segment_id, at_turn_index)` のみ）。妥当性を以下で評価:
- もし型レベルで保証するなら `SegmentOrigin { session_id, segment_id, at_turn_index }` にし、`SegmentStart.session_id == forked_from.session_id` の比較を `Deserialize` 後に走らせる方向、あるいは phantom-typed の compile-time invariant が必要。前者は redundant な persistence、後者は Rust の表現力で書くのが重く割に合わない。
- 現実問題、`SegmentOrigin` の構築点は `create_compacted_segment` / `fork_at` / `ensure_segment_head` の 3 経路に限定され、いずれも source の session_id を継承するルートのみ通る。新規 callsite が追加されない限り invariant は破れない。CLAUDE.md の「設計を歪めない」原則と整合する範囲では、現状の構築時保証 + doc comment 明示で実用上十分。
- ticket 要件文言「型レベルで保証」は厳密にはアンメットだが、実装方針として **「同 Session 性は構築時保証、doc で invariant 明示」** に倒した判断は妥当。ticket 文言を改めるか、現状を accept するかは user 判断。
- **`Pod::from_manifest_spawned` の session_id 新規発行**: spawn された child Pod が親と独立した Session を持つのは、conversation grouping の意味論として妥当（child Pod は別 conversation を成す）。ただし spawn 元の親子関係を Session 永続表現が持たないと、後で「child の Session を親の Segment と紐付ける」操作（spawn linkage）が必要になる可能性がある。範囲外。
- **`lookup_session_of` の O(N) ディレクトリ走査**: shim 用途のみで、pod-cli 起動時に 1 回、TUI resume 時に 1 回呼ばれる程度。Session 数が極端に多い（10^5 オーダー）と起動時 latency に効くが、ユーザーが実際に保持する Session 数（典型 10^2 オーダー）では問題にならない。picker は使わない（picker は session_id を直接持っている）。実装はディレクトリ存在 check で短絡 (`fs_store.rs:147`) しているので最悪ケースも軽い。妥当。
- **TUI picker の Session 単位列挙への変更**: 以前は Segment 単位列挙 → 各 row 1 segment。現在は Session 単位 + 各 Session の leaf segment（最新）のみ preview。compaction で複数 Segment ができている Session でも 1 row。これは「同じ会話の家系」をユーザーが認識する単位として正しい UX。ticket の「Session 内の leaf Segment 列挙 API を提供」と整合（picker 経由で実際に leaf を選んで resume している）。
- **過剰実装の確認**: live-fork marker、Pod 単位 metadata、TUI Session/Segment 選択 UI、DB backend は範囲外。いずれも実装に踏み込んでおらず、最小実装に留まっている。
- **依存追加なし**: 既存型 (`uuid::Uuid`、`ArcSwap`) のみで構築。`cargo add` ポリシー違反なし。
## 指摘事項
### Blocking
- なし。
### Non-blocking / Follow-up
- **`SegmentOrigin` の同 Session 性が型レベルでは保証されない**:
`crates/session-store/src/segment_log.rs:181` の `SegmentOrigin` は `segment_id` と `at_turn_index` のみで、`session_id` を持たない。ticket 要件「型レベルで保証」とは厳密に乖離する。
- 実用上は構築点が `create_compacted_segment` / `fork_at` / `ensure_segment_head` に限定され、いずれも source の session_id を継承する経路のみ通るため invariant は維持されているが、`Deserialize` 経由で外部から不整合 payload を流し込めば破れる。
- 厳密化するなら `SegmentOrigin { session_id, segment_id, at_turn_index }` に拡張し、`collect_state` 時に `SegmentStart.session_id` との一致を assert する手も。ただし persistence が冗長になるトレードオフがある。
- 後続 ticket（fork tree の永続表現が増える `live-fork-marker` 等）で扱うか、現状の「構築時保証 + doc 明示」で十分とするかは設計判断。
- **migration 方針がコード上で明示されていない**:
ticket 完了条件「既存 sessions ディレクトリは破棄して良いかどうかをここで明示」が code/doc で明示されていない。
- `crates/session-store/src/fs_store.rs:1-8` の Layout doc に「旧 flat layout (`<root>/<segment_id>.jsonl`) は本実装では認識されず、`list_sessions` は dir entries のみ列挙する。後方互換は意図的に作らない」旨を 1 行足す程度で十分。
- ticket 上は説明されているので、`tickets/session-grouping-introduce.md` を grep して runtime 文脈に書き出す形でも可。
- **`PickerOutcome::Picked.session_id` フィールドが未使用**:
`crates/tui/src/picker.rs:67` で `session_id: SessionId` を返すが、`crates/tui/src/main.rs:213` は `Picked { segment_id, .. }` で `session_id` を捨てている。
- `cargo check` の `field is never read` warning として表面化。
- 現状 TUI resume は `--session <segment_id>` のみを子 pod に渡し、pod-cli 側で `lookup_session_of` で解決し直すフローなので picker が session_id を返してもパススルーされる先がない。
- 解決策: (a) `Picked { segment_id }` のみにする / (b) TUI から子 pod に session_id も渡せるようフラグを足す（pod-cli 側で `lookup_session_of` を省略できる）。short-term は (a)、long-term は (b)。
- 同様に `picker.rs:339` の `short_segment(id: SessionId)` は名前が `short_segment` のまま（表示対象は session_id）。previous review の nit と同類の取り残し。
- **`tools::Tracker` / `TaskStore` の "session-scoped" / "session-lifetime" コメント残り**:
segment-rename 後続コミット (`7577577`) で header コメントは `Pod-lifetime` に統一されたが、内部コメントに `session-scoped` / `session-lifetime` が残存:
- `crates/tools/src/tracker.rs:21` "A `Tracker` is **session-scoped**: ..."
- `crates/tools/src/lib.rs:10,45` "session-lifetime, enforces..." / "(session-lifetime)"
- `crates/tools/src/task.rs:3,254,260,263,266` "Pod/session-lifetime state" / "session-lifetime task"
- `crates/pod/src/pod.rs:803,811,823` "session-scoped file-operation tracker" / "session-scoped TaskStore"
- `crates/pod/src/compact/worker.rs:5` "session-lifetime `Tracker`"
- 本 ticket で **本物の Session 概念**が入った今、これらの comment は誤読を招く（読み手が「これは新 SessionId に紐づくのか？」と疑う）。実体は Pod プロセス寿命なので `Pod-lifetime` / `Pod-scoped` に統一すべき。
- segment-rename の review follow-up として一部対応されたが取り残しが多いので、本 ticket かこの後ろの「pod-lifetime 用語整理」ticket でまとめて清掃するのが良い。
- **`picker.rs:194-212` の `last_message_preview` が `AssistantItems`（旧 plural）のみ match**:
本 ticket の前 (`segment-rename` 時点) からの取り残しだが、新規 write が `AssistantItem` (singular) のみ使う運用に変わったため、picker preview は assistant 発言を拾えなくなり実質「user 発言 or `[empty]`」のみになる。本 ticket で picker UX を Session 単位に変えた今、preview 品質の劣化が表面化しやすい。`LogEntry::AssistantItem`、`LogEntry::SystemItem`、`LogEntry::ToolResult` への対応追加が望ましい。
### Nits
- **`session_test.rs` のテスト名**: `session_run_logs_entries`、`session_restore_round_trip`、`session_resume_after_pause`、`session_run_with_tool_call`、`session_fork_creates_new_session`、`session_fork_at_truncates_within_session`、`session_config_changed_logged`、`session_auto_forks_on_conflict`（`tests/session_test.rs`）。中身は新 `SessionId` を引数で受けて `restore`/`fork` する正当な Session 概念のテストになっているので、名前と中身は整合的。ファイル名 `session_test.rs` も今となっては妥当。previous review の nit からは解消方向。
- **`Pod::from_manifest_spawned` で child Pod が新 Session を起こす意図**: コードコメントが明示していない。`pod.rs:2867` の周辺に「spawn された child は独立した conversation grouping を持つ（親と Session を共有しない）」旨の 1 行を残しておくと、後で「spawn linkage を session 上で表現するか」検討する時の根拠が消えにくい。
- **`Pod::session_id()` の安定性 doc**: `pod.rs:572-576` で「Stable across compaction and in-Session fork — only `fork` changes it」と書かれているが、コード上の `fork()` (brand-new Session の `session_store::fork` を内部で呼ぶ Pod-level API) は実装されていない。`session_store::fork` を直接呼ぶフリー関数のみ存在し、`Pod` の method としては自身の `session_id` を入れ替える操作が今のところ無い（auto-fork も compaction も同 Session 維持）。doc は将来の Pod-level fork を見据えた spec だが、現実装範囲では「`session_id()` は Pod 寿命を通じて不変」と書いた方が現状認識として正確。
- **`SegmentState` のメソッド `set_location` vs 旧 `set_segment_id`**: previous review で `set_session_id` → `set_segment_id` のリネームが follow-up として履行されたが、本 ticket で更に `set_location(SegmentLocation)` に変わった（segment_id + session_id を atomic swap する正しい形）。旧 `set_segment_id` は callsite 全消し済み。整合的。
## 判断
**Approve with follow-up** — 必須要件（`SessionId` 型導入 / Segment 生成パスの session_id 決定 / `(SessionId, SegmentId)` restore / FsStore layout / leaf 列挙 / `cargo check` + 全テスト pass）は満たされ、ticket の最小実装範囲を守っている。
懸念点は (1) `SegmentOrigin` の同 Session 性が「型レベル保証」ではなく「構築時保証 + doc 明示」に留まる点（ticket 文言と乖離するが、実装方針として CLAUDE.md の「設計を歪めない」原則とは整合）、(2) migration policy がコード上で文字に起こされていない点、(3) `PickerOutcome.session_id` の dead field 残り、(4) picker preview の旧 plural-only 対応、(5) tools / pod の "session-scoped" コメント残り。いずれも blocking ではなく、後続 ticket か小さな follow-up commit で清掃可能。
View Git Blame Copy Permalink