Hare/yoi
1
1
Fork 0
Code Actions Packages Releases Activity
d7ff25b6a7
yoi/tickets/segment-rename.review.md
Hare d7ff25b6a7
update: 残存 Session 識別子の Segment 化（review follow-up） 
レビュー指摘の通り、次の session-grouping-introduce で新 SessionId が
入る前に名称衝突を避けるため取り残しを掃除。

- PodError::Session{Empty,ScopeMissing} → Segment{Empty,ScopeMissing}
- ScopeLockError::SessionConflict → SegmentConflict
- Pod.session_state / SegmentState.set_session_id 系
- source_session_id / prev_session_id / ensure_session_head / short_session
- pod_cli の "Session ID:" 表示
- fs_store の sessions ローカル変数
2026-05-20 05:17:49 +09:00

70 lines
9.9 KiB
Markdown
Raw Blame History

# Review: session-store: SessionId → SegmentId へのリネーム
対象コミット:
- `0d7c37f update: SessionId / SessionStart / SessionOrigin 等を Segment 系名称へ`
- `7577577 update: Session-lifetime/scoped を Pod-lifetime に修正`
ベース: `develop` (`d5c7330 Merge: entry-hash-abolish`)
## 前提・要件の確認
- **`SessionId` 型がコードベースに残らない**: 達成。`grep -rn 'SessionId' crates --include='*.rs'` でヒットゼロ。`SegmentId` 型 alias は `crates/session-store/src/lib.rs:56` に定義され、`new_segment_id()` (同 lib.rs:59) も改名済み。
- **`SessionStart` / `SessionOrigin` / `SessionState` / `SessionLogSink` / `SessionLockInfo` 等の segment レベル `Session*` 型を Segment に統一**: 達成。同 grep でこれらの PascalCase 識別子も全て不在。`SegmentStart{,State}` (`session-store/src/segment.rs:18`)、`SegmentOrigin` (`segment_log.rs:175`)、`SegmentState` (`pod/src/pod.rs:60`)、`SegmentLogSink` (`pod/src/segment_log_sink.rs:42`)、`SegmentLockInfo` (pod-registry) に置き換わっている。
- **`Store::{list_segments, create_segment}` / `create_compacted_segment` / `pod_registry::{update_segment, lookup_segment, find_by_segment}` への改名**: 達成。`session-store/src/store.rs:48,51` で trait 名前が更新され、`pod-registry/src/{lifecycle,mutate,table}.rs` の呼び出し元と本体も全て追従。
- **`protocol::Event::SegmentRotated` / `CompactDone.new_segment_id`**: 達成。`crates/protocol/src/lib.rs:383, 411`。`SessionRotated` の grep もヒットゼロ。
- **module / file rename**: `crates/session-store/src/{session.rs → segment.rs}`、`{session_log.rs → segment_log.rs}`、`crates/pod/src/{session_log_sink.rs → segment_log_sink.rs}` が `git mv` で実行され、`pub mod` / `use` 宣言も追従。
- **crate 名 `session-store` の保留**: 維持。`Cargo.toml` の `name = "session-store"` はそのまま。
- **CLI flag `--session` / TUI 内部 `ResumeWithSession` / `InvalidSession` / `~/.insomnia/sessions/` ディレクトリ**: 維持。`crates/tui/src/main.rs:54-99`、`crates/pod/src/main.rs:188`、`manifest::paths::sessions_dir()` 経路は触られていない。defer 方針通り。
- **`cargo check --workspace` と全テスト pass**: 確認。`target/debug` で `Finished dev` まで通り、`cargo test --workspace` も `test result: ok` のみで完走（`double_run_returns_error` の flakiness はチケット冒頭で報告済みの KNOWN_ISSUES 既知別件）。
- **既存 JSONL を Segment 中心 API で読み書きできる**: `LogEntry::SegmentStart` で serde 表記が新しく `kind = "segment_start"` 系に倒れる方向 (Rust の rename 規約に基づく)。旧 JSONL を読みたい場合は migration が要るが、CLAUDE.md 方針「不必要な後方互換は作らない」と整合。新コードで作って読む限りは round-trip する (`session-store/tests/fs_store_test.rs`, `session_test.rs` の全 pass で確認)。
## アーキテクチャ・スコープ
- 機械的 rename のみで、層境界・API 構造に変化なし。llm-worker / pod / pod-registry / protocol / tui の全層で同じ語彙が一斉に移っているので、コードベースの語彙整合性は素直に上がっている。
- 第 2 コミット (`7577577`) の `Session-lifetime` → `Pod-lifetime` 是正は、ticket の rename 趣旨を越えるが、関連語彙の整理として妥当。`tools::TaskStore` / `tools::Tracker` は compaction を跨いで `Pod` プロセス寿命まで生きるという事実関係に doc が即している。むしろ「旧 SessionId=Segment 時代でも `Session-scoped` 表現は不正確だった」点を解消できているので、本チケットの中で片付けた判断は妥当。
- file rename を要求していない要件に対して、`session.rs → segment.rs` 等の module rename まで踏み込んだ判断は、対称性確保として妥当。`pub mod` / `pub use` の更新コストは小さく、ファイル名と中身の概念が乖離する状態を避ける方が長期保守に有利。過剰ではない。
- 次チケット `session-grouping-introduce` で新たに `SessionId` 型 (Segment 群の grouping) が入る予定なので、現状の rename は新型導入と語彙衝突しない土台を提供できている。
## 指摘事項
### Blocking
- なし。
### Non-blocking / Follow-up
- **公開エラー variant が `Session*` のまま残っている**: 以下は segment レベルの条件で発火するため、ticket の rename 方針 (segment レベルを指す Session* 型は Segment に倒す) に従って後続で追従するのが望ましい。次チケットで新 `SessionId` が入ると、これらの variant 名は誤読を招く。
- `PodError::SessionEmpty { segment_id: SegmentId }` (`crates/pod/src/pod.rs:3238`) → `SegmentEmpty`
- `PodError::SessionScopeMissing { segment_id: SegmentId }` (`crates/pod/src/pod.rs:3243`) → `SegmentScopeMissing`
- `ScopeLockError::SessionConflict { segment_id: SegmentId, ... }` (`crates/pod-registry/src/error.rs:33`) → `SegmentConflict`
- 対応する `#[error("session {segment_id} ...")]` メッセージ文字列、`tui/src/spawn.rs:60` の重複文言、テスト (`pod/tests/restore_test.rs:78,79,104,105`, `pod-registry/src/{lifecycle.rs:323-331, mutate.rs:591,603,611}`) も連動。
- **`SegmentState` の setter 名が `set_session_id`**: `crates/pod/src/pod.rs:72`。型 (`SegmentState`) と getter (`segment_id()`) は改名済みだが setter だけ旧名のまま。`set_segment_id` に揃えるのが自然 (呼び出しは `pod.rs:1659, 2163` の 2 箇所のみ)。
- **`Pod` 構造体のフィールド `session_state: Arc<SegmentState>`**: `crates/pod/src/pod.rs:167` および計 12 箇所 (340, 385, 481, 548, 607, 630, 632, 1621, 1659, 1660, 2148, 2163, 2438, 2768, 2838, 2977)。型は `SegmentState` なのにフィールド名が旧概念に固定されている。`segment_state` に揃えるのが整合的。
- **`pod_cli.rs:78-79` の取り残し**: 第 2 コミットで line 57 を `Segment:` に直したが、末尾の `println!("\nSession ID: {}", pod.segment_id())` は据え置き。ユーザー向けサンプルなので意図的なら問題ないが、第 2 コミットの趣旨 (`Session:` → `Segment:`) と非対称。
- **doc comment の旧語彙残り (narrative)**:
- `crates/session-store/src/lib.rs:5` "Sessions are recorded as a sequence of `LogEntry`..." (本文は Segment が主語のはず)
- `crates/session-store/src/segment_log.rs:154-159` "session-store は payload を不透明扱い... 「session 寿命に縛りたいが session-store の型を汚したくない」"
- `crates/pod/src/segment_log_sink.rs:1,36,47,51,107,142,158,170,181` "session-log mirror" / "session log mirror mutex poisoned" 等
- `crates/pod/src/pod.rs:157,164,166` "persists session state via `session-store`" / "Shared session pointer"
- これらは旧チケットの慣性で残っているもの。即時の修正は必須でないが、`session-grouping-introduce` で本物の Session 概念が入ると曖昧度が増す。
### Nits
- **テスト関数名と test ファイル名の取り残し**:
- `crates/session-store/tests/session_test.rs` (ファイル名)。中身は segment 操作のテスト群 (`session_run_*`, `session_restore_round_trip` 等)。次の grouping 導入で本物の session レベルのテストを足す場合に名前が衝突する。
- `crates/session-store/tests/fs_store_test.rs:56,82,135` (`create_session_writes_all_entries`, `list_sessions_returns_newest_first`, `not_found_error_for_missing_session`)。
- `crates/pod-registry/src/lifecycle.rs:258,279,299,320` (`lookup_session_returns_live_writer_info`, `update_session_rewrites_allocation_session_id`, `update_session_rejects_when_target_already_held`)。
- `crates/pod-registry/src/mutate.rs:576` (`register_pod_rejects_session_id_collision`)。
- `crates/pod-registry/src/table.rs:224` (`find_by_session_skips_none_placeholders`)。
- `crates/pod/tests/restore_test.rs:35,58,85` (`restore_from_manifest_rejects_unknown_session`, `_rejects_empty_session_log`, `_rejects_session_without_scope_snapshot`)。
- `crates/pod/tests/compact_events_test.rs:182,217,572` (`system_texts_in_sink_session_start`, `compact_emits_session_start_carrying_summary_and_task_snapshot`, `detached_extract_does_not_fork_session_log`)。
- `crates/pod/tests/system_prompt_template_test.rs:178` (`session_start_state_captures_rendered_prompt`)。
- `crates/pod/src/segment_log_sink.rs:203` (test helper fn `session_start()`)。
- **ローカル変数名の取り残し**: `source_session_id` (`pod.rs:2148, 2438`, `session-store/src/segment.rs:63,74`)、`prev_session_id` (`pod.rs:1621`)、`old_session_id` (`pod.rs:2148`)、`session_id_for_usage` (`controller.rs:433`)、`usage_session_id` (`memory/src/tool/read.rs:42`)、`sessions` (`session-store/src/fs_store.rs:83-98`)。`ensure_session_head` 関数名 (`pod.rs:1619`) も同様。
- **`tui/src/picker.rs:316`, `tui/src/spawn.rs:476`**: `short_session(id: SegmentId)` 関数。短縮表示の対象は SegmentId だが関数名は旧名のまま。CLI flag `--session` 側のユーザー文脈に寄せているなら据え置きでも可。
## 判断
**Approve with follow-up** — 要件 (型 rename 網羅 / build & test pass / defer 対象は据え置き) は全て満たされ、`SessionId` 識別子の根絶も完遂。第 2 コミットの `Pod-lifetime` 是正も無関係な改善ではなく、ticket 趣旨の周辺整理として妥当。
ただし `PodError::SessionEmpty` / `ScopeLockError::SessionConflict` 等の **public な error variant 名** と、`Pod::session_state` フィールド・`SegmentState::set_session_id` メソッドの取り残しは、次チケット `session-grouping-introduce` で新 `SessionId` 型が導入される前に片付けておくのが理想 (放置すると新型と命名衝突して読みにくくなる)。
View Git Blame Copy Permalink