yoi/tickets/submit-tui-completion.review.md

Review: サブミット入力 — TUI 補完 + 型付き atom 化

レビュー対象: develop ブランチ作業ツリー（未コミット差分含む）。 スコープ: チケット tickets/submit-tui-completion.md の Phase 1〜4 全体。

前提・要件の確認

• @ / # / / 入力中の候補ポップアップ: crates/tui/src/input.rs:235-271 の pending_completion_prefix がトリガー検出を担い、crates/tui/src/main.rs:397-417 で popup active 時のキーがオーバーライドされる。要件通り。
• トリガー条件 (sigil 直前 = 行頭/空白/chip atom のみ、sigil 直後スペースで閉じる): pending_completion_prefix 内 leading_ok で判定し、completion_prefix_tests (input.rs:794-885) で 9 ケースカバー。@src/main.rs の / 誤検出回避、@x での閉鎖、foo@bar 非トリガー等、要件のエッジを押さえている。
• 確定で indivisible chip に置換 (Backspace で塊削除): replace_with_* (input.rs:200-221) で drain + 単一 atom 挿入し cursor を chip 直後に置く。chip atom は AtomClass::Chip として既存 word motion / delete_word_before の単位扱いになっており (atom_class、paste_counts_as_one_word テスト)、Paste と同等の挙動。
• submit 時に対応 Segment 変換: submit_segments (input.rs:418-462) が FileRef/KnowledgeRef/WorkflowInvoke を専用 Segment に分けて出す。knowledge_and_workflow_chips_emit_typed_segments でカバー。
• 候補列挙の protocol query (GetHistory パターン踏襲): Method::ListCompletions / Event::Completions を crates/protocol/src/lib.rs に追加、crates/pod/src/ipc/server.rs:91-118 で同ソケットに直接 reply、broadcast 経路なし。event_completions_format_and_default_is_dir / method_list_completions_roundtrip で wire 形を固定。
• ファイル resolver = auto-read 切り出し: crates/pod/src/fs_view.rs を新設し、旧 compact/worker.rs の slice_lines / ReadRequirement と旧 pod.rs の自動再読ロジックを PodFsView::render_auto_read に集約。compact 側は再エクスポート経由で利用 (compact/worker.rs:31)。Pod::compact も PodFsView::new(scoped_fs.clone()).render_auto_read(...) で同経路を通る (pod.rs:1106)。
• Knowledge / Workflow resolver は wire のみ・空応答: ipc/server.rs:108-110 で CompletionKind::Knowledge | Workflow => Vec::new() と分岐済み。
• chip 表示: 入力エリアは Atom::chip() で全 chip 共通 render (input.rs:83-92)、Block::UserMessage は chip_span_for で同色 (ui.rs:473-492)。Paste=Magenta, @=Cyan, #=Green, /=Yellow を input area / Block で一致。
• Event::UserMessage の typed segment 経路: 既存 submit-segment-protocol.md が完結済みで、本チケットは Atom 拡張を載せるのみ。

完了条件 4 項目すべて満たしている。/clear の client-side 分岐は仕様変更でチケット側から削除済み。

アーキテクチャ・スコープ

• fs_view モジュールの境界: 「Pod から見たファイルシステム操作」という単一の責務に絞れており、auto-read と補完列挙が同居しているのは妥当 (どちらも ScopedFs + pwd + scope 上の readable 判定の薄い wrapper)。Knowledge/Workflow resolver は別スロットに将来追加される設計で、PodFsView に詰め込む流れにはなっていない (server.rs の kind マッチで分岐済み)。crates/pod/src/lib.rs 直下に置く粒度も現状の他モジュール (compact, hook, prompt) と整合。
• PodSharedState::fs_view: OnceLock<PodFsView>: 周辺は RwLock<T> 系だが、用途が「controller 起動時に1回 attach、以降 read-only」なので OnceLock の方が意図に合う。set_fs_view が set の戻り値を捨てているのは「unit test が直接生成した state にも噛み合う」コメント通りで適切。Controller 側 (controller.rs:113, 236, 289) で fs_for_view: tools::ScopedFs; を宣言してブロック内で代入し、ブロック後に attach する流れも冗長ではない (fs.clone() 1回のみ)。
• 不必要な抽象化: 見当たらない。FileCandidate, CompletionEntry の二重定義は protocol 層と pod 内部層で意図的に分離されており (protocol は wire、pod 内部は path 構造)、server.rs の map で繋がっている。
• 将来用 dead code: なし。CompletionKind::Knowledge | Workflow は ipc/server.rs で実際にヒットする分岐 (空応答)、is_dir フィールドは popup の見た目に使われている。
• コードベースを歪めていないか: pod 側は単純な抽出、TUI 側は既存 Atom::Paste の扱いをそのまま FileRef/KnowledgeRef/WorkflowInvoke に拡張する素直な拡張で、新規の概念導入は最小限。LLM provider policy / cargo add / クレート命名等の方針には影響しない。

指摘事項

Non-blocking / Follow-up

• is_dir 候補の確定挙動が popup の hint と乖離している — crates/protocol/src/lib.rs の CompletionEntry doc コメント (約 357 行目付近):

  is_dir is meaningful only for the file kind — it lets the TUI keep a trailing / after a directory selection so the user can drill in without re-typing the prefix. と書かれているが、App::confirm_completion (crates/tui/src/app.rs:164-181) は entry.value をそのまま replace_with_file_ref に渡すだけで、ディレクトリの場合に / を保持して入力継続させる経路がない。popup 側 (ui.rs:104-110) は entry.is_dir のとき表示に / を足しているので、ユーザーには「Tab で drill in できそう」に見えるが実際は chip として閉じる。 対応案 (どちらも本チケット範囲外で良い):

  • doc コメントを実装に合わせて「is_dir は popup 表示のヒントのみ」に書き換える
  • もしくは「ディレクトリ確定時は chip にせず @subdir/ のテキストを残してカーソルを末尾に置く」挙動を追加する
• Paste 後に refresh_completion() が呼ばれない — crates/tui/src/main.rs:305-307 の TermEvent::Paste(s) => app.insert_paste(s); には refresh 呼び出しがない。@s の途中でクリップボード貼付した場合、popup state は古い (kind, prefix_start, prefix) のまま残り、次のキーストロークで refresh されるまで 1 フレーム不整合な popup が見える可能性がある。insert_char 系と同様に app.refresh_completion() を呼んで Method を返す形にしておくのが整合的。
• compact/worker.rs:358-364 の slice_lines_handles_offset_and_limit テストが fs_view.rs:201-207 と重複 — 関数自体は fs_view に移動し compact 側は use のみなので、テストもこの移動に合わせて削除して良い (fs_view 側で同一カバレッジが取れている)。

Nits

• pending_completion_prefix 内の self.atoms.get(i.wrapping_sub(1)).filter(|_| i > 0) (input.rs:253) は意図通り動くが、if i == 0 { None } else { self.atoms.get(i - 1) } の方が読みやすい。
• controller.rs:113 の let fs_for_view: tools::ScopedFs; 宣言-後代入パターンは正しいが、fs 自身を最後に消費しているのを fs.clone() を fs_for_view = fs.clone(); で先に取ってから register_tools(tools::builtin_tools(fs, ...)) の方が見た目が単純。ただし現状でもコメントで意図が説明されており、可読性の優劣は微差。

判断

Approve — 完了条件 4 項目を満たし、アーキテクチャ・スコープともに歪みなし。フォローアップ事項はいずれも UX 補足やテスト整理で、ブロックすべきものではない。