ticket: session-grouping-introduce 完了
This commit is contained in:
parent
842e7a3c58
commit
bee41379fa
1
TODO.md
1
TODO.md
|
|
@ -8,7 +8,6 @@
|
|||
- Pod: 任意ターンからの Fork（複数ターン巻き戻しを汎用化） → [tickets/pod-session-fork.md](tickets/pod-session-fork.md)
|
||||
- Pod: Inbound PodEvent ハンドリングの重複を統合 → [tickets/pod-inbound-pod-event-dedup.md](tickets/pod-inbound-pod-event-dedup.md)
|
||||
- 永続化層整理 (Storage)
|
||||
- Session (Segment 群の grouping) 導入 → [tickets/session-grouping-introduce.md](tickets/session-grouping-introduce.md)
|
||||
- live auto-fork の marker 形式確定 → [tickets/live-fork-marker.md](tickets/live-fork-marker.md)
|
||||
- Pod 単位永続化
|
||||
- Pod state backend と FsStore 実装 → [tickets/pod-state-backend.md](tickets/pod-state-backend.md)
|
||||
|
|
|
|||
|
|
@ -1,54 +0,0 @@
|
|||
# session-store: Session（Segment 群の grouping）導入
|
||||
|
||||
## 背景
|
||||
|
||||
`segment-rename` で物理 append-only 単位を `Segment` に揃えた。続けて、ユーザー視点の「同じ会話の家系」を表す **`Session`** を導入する。
|
||||
|
||||
`Session` は fork tree 全体を 1 つにまとめる grouping で、compaction / fork は同 Session 内に新 Segment を生やす操作になる。これにより:
|
||||
|
||||
- Session 数が fork で爆発せず、`WHERE session_id = ?` だけで fork tree 全体が取れる。
|
||||
- resume の指定は `(SessionId, SegmentId)` の組で行える。
|
||||
- pod-persistent-state 側で Pod ↔ Session の対応関係を扱いやすくなる。
|
||||
|
||||
## 要件
|
||||
|
||||
- 新 `SessionId` 型を `session-store` に追加。Segment は `parent_session_id` を持つ。
|
||||
- Segment 生成パスでの session_id 決定:
|
||||
- new session: 新 `SessionId` を発行
|
||||
- compaction: 元 Segment と同 `SessionId` を継承
|
||||
- fork (live auto / 過去 fork いずれも): 元 Segment と同 `SessionId` を継承
|
||||
- `SessionOrigin` を以下のいずれかに整理:
|
||||
- `compacted_from { segment_id, at_turn_index }`
|
||||
- `forked_from { segment_id, at_turn_index }`
|
||||
- どちらも同 Session 内 segment への参照であることを型レベルで保証。
|
||||
- restore API を `(SessionId, SegmentId)` を取る形に。`SegmentId` のみを取る既存経路は内部で `SessionId` を解決する shim を一段噛ませる。
|
||||
- FsStore layout に Session 単位の index を追加（Session → Segment 列挙が `WHERE session_id = ?` 相当で引けること）。形式は `<data_dir>/sessions/<session_id>/<segment_id>.jsonl` または別ファイル index、実装時に決定。
|
||||
- Session 内の leaf Segment 列挙 API を提供（pod-name resume 等から使う）。
|
||||
|
||||
## 完了条件
|
||||
|
||||
- `SessionId` 型と Session 単位 metadata の永続表現が決まり、FsStore で読み書きできる。
|
||||
- compaction / fork が同 Session 内 Segment 増分として記録される。
|
||||
- `(SessionId, SegmentId)` での restore が動作し、leaf 以外を指定した read-only restore も実装上は可能。
|
||||
- 既存 session を Session 単位に grouping する migration 戦略が決まっている（プロジェクト方針として後方互換は作らないため、既存 sessions ディレクトリは破棄して良いかどうかをここで明示）。
|
||||
- `cargo check --workspace` および全テストが通る。
|
||||
|
||||
## 範囲外
|
||||
|
||||
- live auto-fork の marker 形式（別チケット `live-fork-marker`）。
|
||||
- Pod 単位 metadata（Phase 2 一連のチケット）。
|
||||
- TUI からの Session/Segment 選択 UI。
|
||||
- DB backend 実装。
|
||||
|
||||
## 関連
|
||||
|
||||
- `tickets/segment-rename.md` (前提)
|
||||
- `tickets/live-fork-marker.md`
|
||||
- `tickets/pod-state-backend.md`
|
||||
- `crates/session-store/`
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve with follow-up
|
||||
- レビュー詳細: [./session-grouping-introduce.review.md](./session-grouping-introduce.review.md)
|
||||
- 日付: 2026-05-20
|
||||
|
|
@ -1,106 +0,0 @@
|
|||
# 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 で清掃可能。
|
||||
Loading…
Reference in New Issue
Block a user