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
2
3
|
const existing = await getWorktreeForTask(taskId);
if (existing) return fail(`Worktree already exists for task ${shortId}`);
|
問題 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
這是本次 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 |
✅ 符合 |
|
風險評估
- 🔴 高風險：
taskId vs shortId 語義混淆（問題 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
2
3
4
5
6
7
8
| export interface WorktreeInfo {
taskId: string;
shortId: string;
path: string;
branchName: string;
createdAt: string;
status: 'active' | 'removing' | 'orphaned';
}
|
listActiveWorktrees()（第 188-198 行）：因為從目錄名稱無法還原完整 taskId，taskId 欄位填入空字串或 shortId，並在 shortId 欄位填入正確值。
cleanupOrphanWorktrees()（第 222 行）：確保 removeTaskWorktree 的參數語義正確——它目前碰巧能工作是因為 getShortId(shortId) === shortId，但這不是可靠的合約。
測試更新：tests/unit/worktree-manager.test.ts 需相應調整 assertion。
其他項目全數通過：
★ Insight ─────────────────────────────────────
- **TOCTOU（Time-of-Check-to-Time-of-Use）**是並行系統的經典問題。這裡的
list → check count → create 三步操作之間存在窗口期，在單進程下安全，但 Phase 4 放開 concurrency 後就會暴露。常見解法：file lock、atomic counter、或讓 git 自身的錯誤（branch 已存在）充當 guard。
execFile vs exec 的安全差異：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 語義混淆問題