Agent: reviewer (
reviewer)
Confidence: 60%
Cost: $0.4743
Duration: 1m 53s
Task ID:b9410c3b-3f25-4b48-b140-b1b4ad9ecb24
好的，所有 import 路徑和模式都符合專案慣例。現在我有足夠的資訊來撰寫完整的 Code Review 報告。
Worktree Manager Phase 1 — 架構分析報告
概述
src/agents/worktree-manager.ts 實作品質良好，遵循專案慣例（ESM import、Result<T> pattern、logger），execFile（非 exec）選用正確可防止 shell injection。測試覆蓋率合理（16 tests 涵蓋正常路徑、邊界、fallback）。整體是一個乾淨的 Phase 1 交付。但發現 3 個需要修正的問題和 4 個建議改善項。
分析詳情
問題 1：TaskId 碰撞風險 — shortId 只取 8 字元（重要度：3/5）
- 位置：
worktree-manager.ts:43-45 - 問題：
getShortId()只取taskId.slice(0, 8)。如果兩個 UUID 的前 8 字元相同（概率 ~1/4.3 billion per pair，但累積多次後不可忽略），會產生路徑碰撞 →git worktree add失敗（branch 已存在）。 - 評估：
createTaskWorktree的 try/catch 會捕獲 git 錯誤並回傳fail()，所以不會造成數據損壞。但錯誤訊息"Failed to create worktree: branch already exists"會很令人困惑。 - 結論：可接受（Phase 1 不太可能碰到），但建議在
createTaskWorktree開頭加一個 existence check：
1 | // 在 git worktree add 之前 |
問題 2：listActiveWorktrees() → createTaskWorktree() 的 TOCTOU 競爭（重要度：2/5）
- 位置：
worktree-manager.ts:74-79 - 問題：先
listActiveWorktrees()檢查數量，再execGit(worktree add)。兩步之間如果另一個 process 同時建立 worktree，有可能超過 MAX_WORKTREES 限制。 - 評估：Phase 1 尚未整合到 scheduler，且同類 agent 目前有序列化保護（concurrency=1），所以實際不會發生。Phase 4 放開 concurrency 時需要加 file-based lock 或 atomic counter。
- 結論：Phase 1 可接受，Phase 4 前必須解決。 建議在 spec 或 TODO 中記錄。
問題 3：listActiveWorktrees() 回傳的 taskId 是 shortId，而非完整 UUID（重要度：4/5）
- 位置：
worktree-manager.ts:188, 197-198 - 問題：
listActiveWorktrees()從目錄名稱解析出的taskId只是shortId（8 字元），但WorktreeInfo.taskId的型別是string，語義上應該是完整的 taskId。這導致：cleanupOrphanWorktrees()呼叫removeTaskWorktree(wt.taskId)時傳入 shortId →getShortId()再 slice 一次只取前 8 字元 → 剛好碰巧正確（8 字元 slice(0,8) 仍是 8 字元）。- 但語義上很脆弱——如果 shortId 長度改變，整個鏈路會斷。
- 註釋
// Note: this is the shortId, not the full taskId已經承認了這個問題。
- 結論：必須修正。 要麼：
- (A)
WorktreeInfo增加shortId欄位，taskId在無法還原時設為 shortId 但明確標記 - (B) 改名為
shortId而非混用taskId
- (A)
這是本次 review 最重要的問題——型別系統沒有保護到這個語義差異，未來 Phase 2 整合 scheduler 時很容易踩坑。
問題 4：removeTaskWorktree 的外層 try/catch 可能是死碼（重要度：2/5）
- 位置：
worktree-manager.ts:130-153 - 分析：外層
try/catch包裹了整個函數體，但內部已經有：- git worktree remove 的 try/catch（fallback 到 rm + prune）
- git branch -d 的 try/catch（容忍失敗）
logger.info是 async 但不太可能 throw
- 結論：外層 catch 實際上只會捕獲
rm()或execGit(['worktree', 'prune'])在 fallback 路徑中的異常。這是合理的防禦編程，保留。
測試品質評估
測試檔：tests/unit/worktree-manager.test.ts（260 行，16 tests）
| 面向 | 評分 | 說明 |
|---|---|---|
| Mock 設定 | ✅ 良好 | vi.mock 在 top-level，mock node:child_process、node:fs/promises、logger |
| 覆蓋率 | ✅ 良好 | 正常路徑 + 錯誤路徑 + fallback + 邊界條件 |
| 隔離性 | ✅ 良好 | beforeEach 清 mock，不依賴磁碟 |
| 缺少的測試 | ⚠️ | 1) 未測試 symlink 目標路徑是否正確（只驗了呼叫次數） 2) 未測試 getShortId 對非 UUID 輸入的行為3) 未測試 cleanupOrphanWorktrees 呼叫 removeTaskWorktree 時傳入的是 shortId 而非完整 taskId |
與架構 Spec 的符合度
| Spec 要求 | 實作狀態 | 備註 |
|---|---|---|
createTaskWorktree() 建立 worktree + symlinks |
✅ 完全符合 | |
removeTaskWorktree() 清理 + fallback |
✅ 完全符合 | |
listActiveWorktrees() |
✅ 符合 | taskId/shortId 語義問題需修正 |
cleanupOrphanWorktrees() |
✅ 符合 | |
Worktree 放 /home/arc/worktrees/ |
✅ 符合 | |
| Symlink: soul, node_modules, data, .env | ✅ 符合 | |
| MAX_WORKTREES = 4 | ✅ 符合 | |
| TTL = 2 小時 | ✅ 符合 | |
使用 execFile（非 exec） |
✅ 符合（超越 spec） | Spec 用 exec，實作更安全 |
| 不改變現有行為 | ✅ 確認 | 無其他檔案 import worktree-manager |
Result<T> pattern |
✅ 符合 | ok()/fail() 正確使用 |
| ESM import | ✅ 符合 |
風險評估
- 🔴 高風險：
taskIdvsshortId語義混淆（問題 3）——Phase 2 整合時會產生 bug - 🟡 中風險：TOCTOU 競爭（問題 2）——Phase 4 前需加 lock
- 🟢 低風險：shortId 碰撞（問題 1）——機率極低，且有 error handling
驗收結論
未通過的驗收項目
問題 3（taskId/shortId 語義混淆）必須修正後才能通過。
具體修正要求：
檔案：src/agents/worktree-manager.ts
WorktreeInfo介面修改（第 33-39 行）：將taskId改名為shortId，或新增獨立的shortId欄位。建議方案：
1 | export interface WorktreeInfo { |
listActiveWorktrees()（第 188-198 行）：因為從目錄名稱無法還原完整 taskId，taskId欄位填入空字串或 shortId，並在shortId欄位填入正確值。cleanupOrphanWorktrees()（第 222 行）：確保removeTaskWorktree的參數語義正確——它目前碰巧能工作是因為getShortId(shortId) === shortId，但這不是可靠的合約。測試更新：
tests/unit/worktree-manager.test.ts需相應調整 assertion。
其他項目全數通過：
-
src/agents/worktree-manager.ts存在且編譯通過 -
tests/unit/worktree-manager.test.ts存在且測試通過 -
/home/arc/worktrees/目錄已建立 - 不影響任何現有檔案（純新增）
-
npx tsgo --noEmit全專案編譯通過 - 全部 1108 個測試通過，零回歸
- ❌
taskId/shortId語義必須釐清
★ Insight ─────────────────────────────────────
- **TOCTOU（Time-of-Check-to-Time-of-Use）**是並行系統的經典問題。這裡的
list → check count → create三步操作之間存在窗口期，在單進程下安全，但 Phase 4 放開 concurrency 後就會暴露。常見解法：file lock、atomic counter、或讓 git 自身的錯誤（branch 已存在）充當 guard。 execFilevsexec的安全差異：exec會 spawn shell 並解析字串（容易被 shell injection），execFile直接 fork + execve，不經過 shell parsing。這是正確的防禦做法，且 programmer 超越了 spec 的要求（spec 寫的是exec）。- 語義型別 vs 結構型別：TypeScript 的
string無法區分 “完整 UUID” 和 “8 字元 short ID”。這正是 branded types（type TaskId = string & { __brand: 'TaskId' }）要解決的問題。Phase 1 不需要上 branded types，但至少欄位命名要清晰。─────────────────────────────────────────────────
結論：退回修正 taskId/shortId 語義問題。修正範圍小（介面 + 2-3 處賦值），不影響整體架構。修正後重新送審即可通過。
—HANDOFF—
TO: programmer
INTENT: feedback
ARTIFACT_TYPE: review
SUMMARY: Phase 1 worktree-manager 整體良好，退回修正 taskId/shortId 語義混淆問題