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 のコメント補強
This commit is contained in:
parent
5edc4d3b03
commit
0dfdd11921
|
|
@ -570,7 +570,9 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
}
|
||||
|
||||
/// The Session this Pod belongs to. Stable across compaction and
|
||||
/// in-Session fork — only `fork` (a brand-new Session) changes it.
|
||||
/// auto-fork (both stay within the same Session); there is no
|
||||
/// Pod-level operation today that moves a running Pod to a different
|
||||
/// Session.
|
||||
pub fn session_id(&self) -> SessionId {
|
||||
self.segment_state.session_id()
|
||||
}
|
||||
|
|
@ -2864,6 +2866,8 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
let mut common = prepare_pod_common(&manifest, &loader, /* parse_template */ true)?;
|
||||
let skill_shadows = std::mem::take(&mut common.skill_shadows);
|
||||
|
||||
// A spawned child starts its own conversation, so it mints a
|
||||
// fresh Session rather than joining the spawner's.
|
||||
let session_id = session_store::new_session_id();
|
||||
let segment_id = session_store::new_segment_id();
|
||||
let scope_allocation = pod_registry::adopt_allocation(
|
||||
|
|
|
|||
|
|
@ -6,6 +6,14 @@
|
|||
//!
|
||||
//! The per-Session directory makes `list_segments(session_id)` an O(dir)
|
||||
//! scan and gives the fork tree a visible grouping in the filesystem.
|
||||
//!
|
||||
//! Migration: this layout is incompatible with the pre-`session-grouping`
|
||||
//! flat `{root}/{segment_id}.jsonl` form. Project policy is no
|
||||
//! backward compatibility — discard `~/.insomnia/sessions/` (or whatever
|
||||
//! `root` resolved to) before running the new code. `list_sessions`
|
||||
//! ignores top-level files outside session directories, so leftover
|
||||
//! flat files do not corrupt new sessions, but they are no longer
|
||||
//! enumerable by the picker.
|
||||
|
||||
use crate::event_trace::TraceEntry;
|
||||
use crate::segment_log::LogEntry;
|
||||
|
|
|
|||
|
|
@ -4,12 +4,12 @@
|
|||
//! `llm-worker` `Tool` infrastructure. Filesystem access is mediated by
|
||||
//! two orthogonal concerns:
|
||||
//!
|
||||
//! - [`ScopedFs`] — pod-lifetime, expresses the write-block boundary for
|
||||
//! the current scope. Derived from the manifest and shareable across
|
||||
//! sessions.
|
||||
//! - [`Tracker`] — session-lifetime, enforces the "read before edit"
|
||||
//! - [`ScopedFs`] — Pod-process lifetime, expresses the write-block
|
||||
//! boundary for the current scope. Derived from the manifest; not
|
||||
//! persisted across Pod restart.
|
||||
//! - [`Tracker`] — Pod-process lifetime, enforces the "read before edit"
|
||||
//! policy via content hashes and tracks the recency of touched files.
|
||||
//! Recreated fresh per session.
|
||||
//! Recreated fresh on each Pod start (including resume).
|
||||
//!
|
||||
//! The Pod layer owns both instances and passes them to
|
||||
//! [`builtin_tools`] when registering tools on a `Worker`.
|
||||
|
|
@ -42,11 +42,11 @@ pub use tracker::Tracker;
|
|||
pub use write::write_tool;
|
||||
|
||||
/// Register all builtin tools, wiring them to a shared `ScopedFs`
|
||||
/// (pod-lifetime) and `Tracker` (session-lifetime).
|
||||
/// (Pod-process lifetime) and `Tracker` (Pod-process lifetime).
|
||||
///
|
||||
/// All returned factories share the same tracker instance so that
|
||||
/// `Read` / `Write` / `Edit` see a consistent history across tool
|
||||
/// invocations within a single session.
|
||||
/// invocations within a single Pod run.
|
||||
///
|
||||
/// `bash_output_dir` is where the Bash tool spills long outputs. The
|
||||
/// caller is responsible for adding that path to the readable scope
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
//! Pod-lifetime TaskStore and builtin task tools.
|
||||
//! Session-lifetime TaskStore and builtin task tools.
|
||||
//!
|
||||
//! The store is Pod/session-lifetime state shared by the four Task* tools. It
|
||||
//! is reconstructed on resume by replaying TaskCreate / TaskUpdate tool-call
|
||||
//! arguments from persisted history.
|
||||
//! The store survives compaction and Pod restart — it is reconstructed
|
||||
//! on resume by replaying TaskCreate / TaskUpdate tool-call arguments
|
||||
//! from persisted history, so its effective lifetime is the
|
||||
//! [`session_store::SessionId`] (the conversation), not the Pod process.
|
||||
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
|
|
|
|||
|
|
@ -18,11 +18,13 @@
|
|||
//!
|
||||
//! # Lifetime
|
||||
//!
|
||||
//! A `Tracker` is **session-scoped**: the Pod layer creates a fresh
|
||||
//! instance at the start of each agent session and discards it when the
|
||||
//! session ends. The `ScopedFs` write boundary, by contrast, is
|
||||
//! pod-lifetime (derived from the manifest). The two are orthogonal and
|
||||
//! the Pod wires them together when registering builtin tools.
|
||||
//! A `Tracker` is **Pod-process scoped**: the Pod layer creates a fresh
|
||||
//! instance at the start of each Pod run (including resume) and discards
|
||||
//! it when the process exits — it is not persisted, so a resumed
|
||||
//! conversation starts with an empty read/edit history. The `ScopedFs`
|
||||
//! write boundary is likewise Pod-process scoped (derived from the
|
||||
//! manifest). The two are orthogonal and the Pod wires them together
|
||||
//! when registering builtin tools.
|
||||
//!
|
||||
//! ```no_run
|
||||
//! # use std::path::PathBuf;
|
||||
|
|
|
|||
|
|
@ -210,7 +210,7 @@ async fn run_resume() -> Result<(), Box<dyn std::error::Error>> {
|
|||
// viewport before the name dialog opens so each phase gets fresh
|
||||
// vertical room.
|
||||
let leaf_segment_id = match picker::run().await? {
|
||||
PickerOutcome::Picked { segment_id, .. } => segment_id,
|
||||
PickerOutcome::Picked { segment_id } => segment_id,
|
||||
PickerOutcome::Cancelled => return Ok(()),
|
||||
};
|
||||
run_spawn(Some(leaf_segment_id)).await
|
||||
|
|
|
|||
|
|
@ -62,11 +62,10 @@ impl From<session_store::StoreError> for PickerError {
|
|||
}
|
||||
|
||||
pub enum PickerOutcome {
|
||||
/// User picked a session; resume at its leaf segment.
|
||||
Picked {
|
||||
session_id: SessionId,
|
||||
segment_id: SegmentId,
|
||||
},
|
||||
/// User picked a session; resume at its leaf segment. The pod-cli
|
||||
/// rehydrates `session_id` via `Store::lookup_session_of` so we only
|
||||
/// need to surface the segment here.
|
||||
Picked { segment_id: SegmentId },
|
||||
Cancelled,
|
||||
}
|
||||
|
||||
|
|
@ -134,10 +133,8 @@ pub async fn run() -> Result<PickerOutcome, PickerError> {
|
|||
}
|
||||
Some(Action::Submit) => {
|
||||
close_viewport(&mut terminal)?;
|
||||
let row = &rows[selected];
|
||||
return Ok(PickerOutcome::Picked {
|
||||
session_id: row.session_id,
|
||||
segment_id: row.leaf_segment_id,
|
||||
segment_id: rows[selected].leaf_segment_id,
|
||||
});
|
||||
}
|
||||
Some(Action::Cancel) => {
|
||||
|
|
@ -200,6 +197,11 @@ fn last_message_preview(entries: &[LogEntry]) -> Option<String> {
|
|||
return Some(format!("user: {}", trim_one_line(&text, 60)));
|
||||
}
|
||||
}
|
||||
LogEntry::AssistantItem { item, .. } => {
|
||||
if let Some(text) = first_text_logged(item) {
|
||||
return Some(format!("assistant: {}", trim_one_line(&text, 60)));
|
||||
}
|
||||
}
|
||||
LogEntry::AssistantItems { items, .. } => {
|
||||
if let Some(text) = items.iter().find_map(first_text_logged) {
|
||||
return Some(format!("assistant: {}", trim_one_line(&text, 60)));
|
||||
|
|
|
|||
|
|
@ -46,3 +46,9 @@
|
|||
- `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
|
||||
|
|
|
|||
106
tickets/session-grouping-introduce.review.md
Normal file
106
tickets/session-grouping-introduce.review.md
Normal file
|
|
@ -0,0 +1,106 @@
|
|||
# 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