64 lines
7.6 KiB
Markdown
64 lines
7.6 KiB
Markdown
# Review: メモリ機構: memory / Knowledge 検索ツール
|
||
|
||
## 前提・要件の確認
|
||
|
||
### ツール構成
|
||
- MemorySearch / KnowledgeSearch を新規 Tool として実装、単一取得は MemoryRead に役割分離 — `crates/memory/src/tool/search.rs`、`tool/mod.rs:21-24` で `read_tool` / `write_tool` / `edit_tool` / `memory_search_tool` / `knowledge_search_tool` を re-export。チケット冒頭の「slug 完全一致は MemoryRead 側」という補正が公開 API・ドキュコメントどちらにも反映されている (`search.rs:31-40`, `read.rs:17-20`)。
|
||
|
||
### Read/Write/Edit signature 補正（memory-file-format からの繰越）
|
||
- `MemoryToolKind` を `tool/mod.rs:28-90` に集約し、Read/Write/Edit 全部で `kind` + `slug` 入力に切り替え済み。`Workflow` を意図的に enum から外しているため、`workflow` を投げた瞬間 deserialize で弾ける（`write.rs:230-243`, `edit.rs:240-253` のテストで担保）。Search 出力 `{slug, kind}` をそのまま Read/Edit に渡せる経路は完了条件どおり成立。
|
||
- summary は `slug` 禁止、その他は必須、というルールを `MemoryToolKind::resolve_path` に一箇所で集約しており、3 ツール全部が同じ振る舞いになる。テストも `summary_with_slug_rejected` / `decision_without_slug_rejected` で担保 (`read.rs:172-187`).
|
||
|
||
### MemorySearch 仕様
|
||
- 入力 `query: string` のみ、case-insensitive 部分一致 — `validate_query` で空文字拒否 + lower-case 化 (`search.rs:224-231`)、`scan_text` で行ごとに lower-case 化して比較 (`search.rs:289-300`)。
|
||
- 出力 `{slug, kind, excerpt}[]`、summary は slug を `"summary"` 固定で返す — `search.rs:115-122` と `memory_search_finds_summary` テスト。
|
||
- 対象/除外ディレクトリ: `summary_path` / `decisions_dir` / `requests_dir` のみ列挙し、`workflow_dir` / `staging_dir` はそもそも触らない構造。`memory_search_excludes_workflow_and_staging` テストで「needle を含めても 0 件」を担保。
|
||
|
||
### KnowledgeSearch 仕様
|
||
- `query` 必須 + `kind` 任意フィルタ。frontmatter が壊れていても本文 grep を継続、frontmatter フィールドは `None` を返す動作 (`search.rs:182-213`)。`knowledge_search_searches_frontmatter_too` で frontmatter 文字列にもヒットすることを担保。
|
||
- 出力スキーマ差別化（description / model_invokation を持つ）はチケット改訂版にも記述あり、原チケットの「同型」を読み手にも明示している。
|
||
|
||
### 共通・ソート・上限
|
||
- `list_md_files` がスラグ昇順ソート (`search.rs:256`)、ファイル内は `lines().enumerate()` で出現順 — チケットの「ファイル名昇順 → 行順」と一致。
|
||
- `manifest [memory]` から `search_hit_limit` / `search_excerpt_lines` を tune できる経路あり (`crates/manifest/src/lib.rs:54-64`, `config.rs:204-212`, `controller.rs:249-260`)。デフォルト 20 / 3 行で原仕様と一致。
|
||
- `memory_search_respects_hit_limit` / `memory_search_excerpt_includes_context_lines` テストで両方が効くことを担保。
|
||
|
||
### 登録
|
||
- `pod/src/controller.rs:256-260` で memory enabled 時にのみ 5 ツールが並列登録される。Phase 2 Pod 側はチケットでも明示的に範囲外と書かれていて整合。
|
||
|
||
### 完了条件マッピング
|
||
- 通常 Pod から 5 ツール呼べる ✅ (`controller.rs`)
|
||
- Search → Read 経路成立 ✅（共通 `MemoryToolKind` で型一致）
|
||
- frontmatter 含む全文ヒット ✅ (`scan_text` は raw 全体を走査、専用テストあり)
|
||
- `kind` filter ✅ (`knowledge_search_kind_filter` テスト)
|
||
- 対象外ディレクトリ非ヒット ✅
|
||
- tuning 可 ✅
|
||
|
||
## アーキテクチャ・スコープ
|
||
|
||
- `crates/memory/src/tool/` にファイル分割で同居、既存 read/write/edit と完全に同じパターン。`MemoryToolKind` を `mod.rs` に置いて 4 ツール全部が共有しているのは適切。
|
||
- 依存追加なし（`std::fs` / `serde_yaml` / 既存の `schema::*` のみ）。`cargo add` ルールに抵触しない。
|
||
- `ToolDefinition` を `Arc<Fn>` で返す既存ファクトリ規約を踏襲し、`SearchConfig` を closure に move するだけのミニマルな受け渡し。
|
||
- `manifest::MemoryConfig` への 2 フィールド追加は `Option<usize>` + cascade で merge する既存パターンに従っている。
|
||
- スコープ越境なし（llm-worker / pod / scope の責務境界に変更なし）。「memory crate は self-contained」という既存ポリシー (`lib.rs:1-7`) もそのまま守られている。
|
||
- 派生 index を持たず都度スキャンとしている点も、原チケットの「FTS / vector は将来」とブレなし。
|
||
|
||
## 指摘事項
|
||
|
||
### Blocking
|
||
なし。
|
||
|
||
### Non-blocking / Follow-up（任意）
|
||
- **`search.rs:115-122` の summary 走査が `hit_limit` 上限ガードを通っていない**。現状は `limit - hits.len()` を `scan_file` に渡しているので `limit == 0` でも 0 が伝わって動作上は問題ないが、可読性として decisions / requests と同じ `if hits.len() < limit` ガードを前置きしておく方が一貫し、将来の修正で誤って `usize` underflow を踏み込む隙を消せる。
|
||
- **`SearchConfig` 適用ロジックが controller 側に展開コードとして書かれている** (`controller.rs:249-255`)。`MemoryConfig` → `SearchConfig` の変換を `From` impl で memory crate 側に持たせるとテストしやすく、controller を細くできる。manifest を memory に依存させたくない場合は逆向き (`manifest` 側に `to_search_config`) でも可。今のままでも動くので任意。
|
||
- **frontmatter の YAML パース失敗時、`kind` filter 指定があるとそのファイルが完全に excluded になる** (`search.rs:189-198`)。仕様上「frontmatter parse 失敗時は本文 grep 継続」と書かれているので、kind filter 指定時のみ skip するのは spec 通り（filter で絞り込みたいユーザーが unknown-kind のファイルを取りたいケースは無い）。ただしドキュコメントに「kind filter ありで frontmatter 壊れているファイルは skip」と一行足しておくと混乱しない。
|
||
- **`KnowledgeSearchTool` は frontmatter parse 失敗ファイルも逐次 lower-case 比較するため負荷がやや高い**。本チケットの「都度スキャン」「FTS は将来」前提なので問題視はしないが、`hit_limit` を 0 や巨大値にしたユーザーに対して description などに足切り入れる余地は将来検討。
|
||
|
||
### Nits
|
||
- `MEMORY_SEARCH_DESCRIPTION` / `KNOWLEDGE_SEARCH_DESCRIPTION` が `hit_limit` に言及しているが、tool input にはこのフィールドが無い（manifest 側 tunable）。LLM が `hit_limit` を引数に入れたくならないか軽く心配。「(configurable via manifest)」のような括弧書きで明示してもよい。
|
||
- `parse_hits` テストヘルパーが `OwnedMemoryHit` / `OwnedKnowledgeHit` で「Owned」プレフィックスを持つが、テスト内のシリアライズ用 struct を「Owned」と呼ぶ意図が明示されていない。`parsed_*` などでも可。
|
||
|
||
## 判断
|
||
|
||
**Approve（完了可）** — 原チケットの要件と完了条件はすべて実装に対応付けが取れており、補正（Read/Write/Edit signature 切り替え・出力スキーマ差別化・slug 完全一致を Read 側へ移譲）はチケット本文にも明確に反映されている。コードベースの既存パターンを踏襲しており、歪みも gold-plating も無し。指摘は全て任意の follow-up。
|