reviewer

Worktree Manager Phase 1 — 架構分析報告

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
// 在 git worktree add 之前
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

  1. WorktreeInfo 介面修改（第 33-39 行）：將 taskId 改名為 shortId，或新增獨立的 shortId 欄位。建議方案：
1
2
3
4
5
6
7
8
export interface WorktreeInfo {
  taskId: string;      // 完整 taskId（建立時傳入的）
  shortId: string;     // 8 字元短 ID（用於路徑和 branch 名稱）
  path: string;
  branchName: string;
  createdAt: string;
  status: 'active' | 'removing' | 'orphaned';
}

  1. listActiveWorktrees()（第 188-198 行）：因為從目錄名稱無法還原完整 taskId，taskId 欄位填入空字串或 shortId，並在 shortId 欄位填入正確值。

  2. cleanupOrphanWorktrees()（第 222 行）：確保 removeTaskWorktree 的參數語義正確——它目前碰巧能工作是因為 getShortId(shortId) === shortId，但這不是可靠的合約。

  3. 測試更新：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 ─────────────────────────────────────

  1. **TOCTOU（Time-of-Check-to-Time-of-Use）**是並行系統的經典問題。這裡的 list → check count → create 三步操作之間存在窗口期，在單進程下安全，但 Phase 4 放開 concurrency 後就會暴露。常見解法：file lock、atomic counter、或讓 git 自身的錯誤（branch 已存在）充當 guard。
  2. execFile vs exec 的安全差異：exec 會 spawn shell 並解析字串（容易被 shell injection），execFile 直接 fork + execve，不經過 shell parsing。這是正確的防禦做法，且 programmer 超越了 spec 的要求（spec 寫的是 exec）。
  3. 語義型別 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 語義混淆問題