Re: spawner実装計画のレビュー
Summary
plannerが作成したspawner実装計画(19c6a0b6bee)を7つの観点からレビューした。前回レビュー(19c69ef076e)で指摘した全Issue/Edge-caseは適切に解消されている。アーキテクチャは全体として妥当だが、実装時に注意すべき点が2件、軽微な指摘が3件ある。
Review Verdict: APPROVED_WITH_NOTES
前回のCHANGES_REQUESTEDの指摘事項がすべて解消されており、計画として十分な品質に達している。以下のNOTEは実装フェーズで対応すれば良い。
Detailed Review
1. ownerの指示(19c66af7608)との整合性: OK
ownerの指示に含まれる全要件(npm run spawner起動、inbox監視、owner除外、1メモ1エージェント、PM単一インスタンス制約、PM再起動条件、全停止時PM起動、Ctrl-Cシャットダウン、強制終了、SPAWNER_SPAWN_CMD、プロンプトファイル、$INPUT_MEMO_FILES、ログ出力、リトライ3回、ドキュメント最適化)がすべて計画に反映されている。
2. 前回レビュー(19c69ef076e)の指摘事項: 全件解消
- ISSUE-1〜5: すべて具体的な解決方針が記載されている
- EDGE-1〜5: すべて対応策が明記されている
- SEC-1: shell:falseで対応
- SEC-2: Step 1でgitignore追加
- SUGGEST-1,2: スコープ外として合理的に先送り
- SUGGEST-3: 採用済み
- SUGGEST-4: 明確化済み
3. アーキテクチャの妥当性
[NOTE-1] SPAWNER_SPAWN_CMDのパースとshell:falseの整合性(重要)
計画ではshell:falseでchild_process.spawnを使用するとあるが、SPAWNER_SPAWN_CMDのデフォルト値が「claude」(-pなし)で、プロンプトは「-p引数として渡す」と記載されている。
問題点:
- shell:falseの場合、SPAWNER_SPAWN_CMDが「claude -p」のようにスペース区切りの文字列だと、そのままではspawnの第1引数に渡せない。コマンド文字列をスペースで分割してcommand/argsに分ける処理が必要。
- ownerの仕様では「SPAWNER_SPAWN_CMDにセットされたシェルコマンドを使う」「デフォルトのclaude -pコマンドが使用される」とある。つまりデフォルトは「claude -p」全体がコマンドであり、プロンプト文字列はその後ろの引数として渡される想定。
- 計画の「プロンプトは-p引数として渡す」という記述は、デフォルトコマンドが「claude」で、spawnerが「-p」フラグを付加する意味にも読める。どちらの解釈かを明確にすべき。
提案: builderへの指示で以下を明記する。
- SPAWNER_SPAWN_CMDはスペースで分割し、先頭をcommand、残りをargsとする
- プロンプト文字列はargsの末尾に追加する
- デフォルトは command=\claude, args=[-p, promptString] とする
[NOTE-2] watcherの起動タイミングのレースコンディション
spawner起動時にまずinboxをチェックし、その後watcherを開始する。この間(チェック完了〜watcher開始)にinboxに到着したメモは検出されない可能性がある。実用上は極めて稀だが、以下で回避可能:
- watcherを先に開始してからinboxの初期チェックを行う
- builderへの実装指示に含めることを推奨
4. テスト計画の網羅性: 概ね十分
テスト計画はユニットテスト・統合テスト・手動テストの3層構成で適切。
[NOTE-3] キューメカニズムのテスト欠如
EDGE-1対応として同時プロセス上限(10)とキュー管理が設計されているが、テスト計画のテーブルにキュー関連のテスト項目が明示的に含まれていない。Step 4の統合テストでカバーされる想定だと思われるが、以下のテストケースを明示すべき:
- 上限到達時にキューに入ること
- プロセス終了時にキューから取り出して起動すること
- キュー内の順序(FIFO等)
5. Builder分担と依存関係: OK
Builder A(Step 1,2,4)とBuilder B(Step 3,5,6)の分担は合理的。依存関係グラフも正しい。Step 2と3の並列実行、Step 5とStep 4の並列実行が可能な設計は効率的。
6. リスク・見落とし
[NOTE-4] エージェントプロセスのタイムアウト未定義
エージェントがハングした場合(無限ループ等)、spawnerは終了モードでも永久に待ち続ける。初期実装ではスコープ外で問題ないが、将来課題として記録すべき。
[NOTE-5] キューの優先順位未定義
EDGE-1で上限到達時にキューに入れるとあるが、キュー内の順序(FIFO、ロール優先度等)が未定義。FIFOで十分だが明記すべき。
7. Constitution準拠: OK
spawnerは内部インフラであり、サイトコンテンツや訪問者体験に直接影響しない。constitution.mdの4つのルールすべてに違反なし。
Summary Table
| 項目 | 判定 |
|---|---|
| ownerの指示との整合性 | OK |
| 前回レビュー指摘の解消 | 全件OK |
| アーキテクチャ妥当性 | OK(NOTE-1,2は実装時対応) |
| テスト計画網羅性 | OK(NOTE-3は追加推奨) |
| Builder分担 | OK |
| リスク・見落とし | NOTE-4,5は将来課題として記録 |
| Constitution準拠 | OK |
Next actions
- project-managerはNOTE-1(SPAWNER_SPAWN_CMDのパース方式)をbuilderへの指示に明記する
- project-managerはNOTE-2(watcher起動順序)をbuilderへの指示に含める
- project-managerはNOTE-3(キューテスト)をテスト計画に追加する
- NOTE-4,5は将来課題としてバックログに記録する
- 上記を反映の上、builderへの実装委任を進めてよい