# Reviewer 審查 M8-4b 補丁（2026-04-15）

## 摘要

- 審查對象：M8-4b 補丁（3 Major + Stage 4 timeout 修復）
- 審查檔案：`visiona-local/startup_pipeline.go`（L385-423 新增 helpers）、`visiona-local/server_control.go`（L167-223 startInternal 守門、L630-652 probe timeout、L867-937 RestartStartupSequence + test hook）、`visiona-local/app.go`（L109-119 struct、L1676-1693 openBrowser var）、`visiona-local/startup_pipeline_test.go`（L393-660 4 個 regression test）
- 總結論：**✅ 通過** — 3 Major 全部正確修復、Stage 4 timeout 改好、4 個 regression test 完整覆蓋且全綠。Build/Vet/Test/Race（count=2）在 `visiona-local` 與 `server` 兩個 module 都 PASS。
- 3 Major 修復狀況：M-1 ✅ / M-2 ✅ / M-3 ✅
- 問題統計：Critical 0 / Major 0 / Minor 2 / Suggestion 1
- 阻擋 M8-10 嗎：**否**，可推進 M8-10。

---

## A. HasFailedStage helper 正確性

位置：`visiona-local/startup_pipeline.go` L385-404

- 邏輯正確 ✅：`current == -1`（`FailStage` 會把 current 設為 -1）或迴圈檢查 `stages[1..6].status == "failed"`（defensive path）。
- Lock 保護 ✅：`p.mu.Lock()` + defer `Unlock()`。`mu` 在 struct 是 `sync.Mutex`（L98），沒 RLock 可用 — 任務敘述提到的「RLock」不適用，用 `Lock` 是正確選擇。效能影響可忽略（只在 startInternal error 路徑讀一次）。
- 文件註解清楚 ✅：L386-391 解釋語義與對 M-1 的作用。
- Test 覆蓋 4 case ✅：`TestStartupPipeline_HasFailedStage`（L537-566）分別驗證：new / running / FailStage 後（current=-1）/ 某 stage status=failed 但 current 未同步（defensive）。

---

## B. IsInColdStart helper 正確性

位置：`visiona-local/startup_pipeline.go` L406-423

- 邏輯正確 ✅：`current >= 1 && current <= startupTotalStages`（startupTotalStages=6），stage 7（ready）與 current=-1（failed）、current=0（未 Start）都回 false。
- Lock 保護 ✅：同 `HasFailedStage`，用 `Lock/Unlock`。
- Test 覆蓋邊界 ✅：`TestStartupPipeline_IsInColdStart`（L578-606）覆蓋 current ∈ {0, 1..6, 7, -1}，迴圈確認 1..6 全 true，其餘全 false。

---

## C. startInternal error 分支守門

位置：`visiona-local/server_control.go` L167-194

- 正確用 `pipeline.HasFailedStage()` ✅（L176-179）。
- `pipeline == nil` fallback ✅：`pipelineHandled` 預設 false → 走原 setState + emit + sendCrashNotification 邏輯。
- 非冷啟動路徑（RestartServer 等，pipeline 已 ready current=7，若此刻 Start 失敗 HasFailedStage=false）fallback 正常發通知 ✅。
- 註解（L168-175）解釋完整：說明為什麼守門 + 哪些路徑仍需 fallback。
- 不破壞 RestartServer（走 `ctrl.Restart()` → `ctrl.Stop()` + `ctrl.Start()`）的錯誤處理 ✅：RestartServer 發生時 pipeline.current==7 或 0，`HasFailedStage()` 回 false。

**小提醒**（非 Major）：此守門建立在「pipeline FailStage 一定早於 startInternal return err」的假設上。實作上 `startServerV2` 每個 stage 失敗都立即 `FailStage`（見 server_control.go L437-448 / L554-566），再 return err，順序正確。若未來新增一條「不透過 FailStage 的錯誤返回路徑」，守門會被繞過。建議記為 Minor 追蹤（見 §L）。

---

## D. openBrowser 守門

位置：`visiona-local/server_control.go` L202-222

- `pipeline.IsInColdStart() == false` 才執行 R5-D3 openBrowser ✅（L213-216）。
- RestartServer 路徑（pipeline.current==7，`IsInColdStart()` 回 false）仍 openBrowser ✅。
- `pipeline == nil` 路徑 `inColdStart` 維持 false，fallback 仍 openBrowser ✅（L214-216）。
- 註解（L204-212）清楚列出三種場景（冷啟動 / RestartServer / RestartStartupSequence）的預期行為，可維護性高。

---

## E. openBrowser package-level var 改動

位置：`visiona-local/app.go` L1676-1693

- 改為 `var openBrowser = openBrowserExec` ✅（L1681）。預設值是實作函式，production 透過 var 呼叫只多一個 indirection，效能影響可忽略。
- 所有呼叫端透過 var 呼叫：`app.go` L268（runStartupStage5）、`server_control.go` L219（startInternal R5-D3）。Test 可替換 var 做 stub。
- `openBrowserExec` 保留為獨立函式（L1684-1693）作為預設實作，macOS/Windows/Linux 三分支原封不動 ✅。
- Test 重置良好 ✅：`TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce` 用 `defer func() { openBrowser = original }()`（L627）保證 test 結束還原。

---

## F. RestartStartupSequence test 重構

位置：`visiona-local/startup_pipeline_test.go` L393-522

- **直接呼叫 `a.RestartStartupSequence()`** ✅（L439）— 不是手動複製 5 步驟。
- 用 `restartStartFn` test hook 攔截 Step 6 ✅（L424-433），spy 記錄呼叫、檢查 sentinel 是否已清、snapshot 新 pipeline。
- `callLog` 驗證順序 ✅（L497-516）：`pipelineCancel` 必須在 `startFn` 之前。
- 驗證新 pipeline 建立 ✅（L466-486）：stage1=completed / stage2=running / current=2。
- `pipelineCancelFn` 的 `context.CancelFunc` 型別允許 assign 任意 `func()` ✅（L407-410）。
- 其他 side effect 驗證完整：proc 被清為 nil（L453-458 ForceKill 效果）、sentinel file 已刪除（L461-463）、啟動前 ctrl state 設為 Error 模擬 Retry 前置條件（L418）。
- 「若未來把 Step 4 移到 Step 2 之前」的概念驗證：test 雖無法直接抓 Step 4 ↔ Step 2 相對順序，但能抓「Step 4 在 Step 6 之前」（`sentinelClearedBeforeStart` L489-491），滿足原 Reviewer 對 M-3 的核心期待（驗證真的呼叫 method、不是手動複製）。

**Minor F-1**（見 §L）：`newPipelineTestApp` 的 `a.ctx == nil`，所以 RestartStartupSequence Step 5 的 watcher goroutine 分支不會啟動，L518-521 的「清理新啟動的 watcher」註解有點誤導（實際上 pipelineCancelFn 已在 Step 1 被清為 nil，之後也沒有被重設）。無功能影響，純註解細節。

---

## G. TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce

位置：`visiona-local/startup_pipeline_test.go` L615-660

- 正確 stub `openBrowser` var ✅（L621-627），defer 還原。
- `AutoOpenBrowser=true`（L618）確保 runStartupStage5 會走 openBrowser 分支（不 skip）。
- `restartStartFn` 內模擬 server 啟動成功：設 fake proc port=12345、setState(Running)、手動推進 pipeline stages 到 stage 5 running ✅（L630-645）。這讓 `runStartupStage5` 能從 `ctrl.proc.port` 取到 URL 並呼叫 openBrowser。
- 斷言 `openCount == 1` ✅（L652-654），錯誤訊息清楚標示 want 1。
- 驗證冷啟動路徑下「startInternal 的 R5-D3 open」不會發生 — 因為 restartStartFn 替換了 `ctrl.Start()`，完全 bypass startInternal，所以實際上這個 test 驗證的是 runStartupStage5 本身只會 open 一次。

**Minor G-1**（見 §L）：此 test 沒有實際經過 startInternal 的 R5-D3 分支（restartStartFn 完全替換 ctrl.Start），所以嚴格來說它驗證的是「runStartupStage5 本身的 open 次數」，而不是「冷啟動中 startInternal + runStartupStage5 的 open 次數總和」。要完整驗證 M-2，需要一個能跑 startInternal 的整合 test（但 startInternal 會 spawn python server 無法 unit test）。現況可接受，因為 IsInColdStart 邏輯已被 B 節 `TestStartupPipeline_IsInColdStart` 單獨驗證，startInternal 的守門條件 `!inColdStart` 是純布林邏輯，靜態即可推得正確。

---

## H. Stage 4 probe timeout 2s

位置：`visiona-local/server_control.go` L630-652

- 註解說明原因 ✅：L634-636 解釋 TDD §3「秒回即算完成」+「2s 足以涵蓋正常 latency」。
- 不破壞既有 test ✅：Stage 4 相關 test 全綠（`go test -count=1` PASS）。
- 逾時仍算完成 ✅（L647-651 不論 err 或 status 都 CompleteStage(4)），符合 TDD §3 設計意圖。

---

## I. Build/Test/Race 結果

| 指令 | 結果 |
|------|------|
| `cd visiona-local && go build .` | PASS（無輸出） |
| `cd visiona-local && go vet ./...` | PASS（無輸出） |
| `cd visiona-local && go test -count=1 ./...` | PASS（8.210s） |
| `cd visiona-local && go test -race -count=2 ./...` | **PASS（16.225s）** |
| `cd server && go build ./...` | PASS |
| `cd server && go vet ./...` | PASS |
| `cd server && go test -count=1 ./...` | PASS（api/handlers/ws/device/model 全通過） |
| `cd server && go test -race ./...` | PASS |

4 個新 test 單獨跑 verbose：
```
--- PASS: TestStartupPipeline_RestartStartupSequence_StepsExecution (0.00s)
--- PASS: TestStartupPipeline_HasFailedStage (0.00s)
--- PASS: TestStartupPipeline_IsInColdStart (0.00s)
--- PASS: TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce (0.00s)
```

---

## J. 合併檢查

`git diff --stat` 看過全部變更，無衝突標記、無重複區塊。合併狀況：

- **M8-4 原版** + **M8-4 補丁**（Stop/ForceKill/handleWatchFailure/logPump）：`ForceKill` L273-291 的 cancelWatcher 順序修復仍在，`RestartStartupSequence` Step 2 正確利用 ✅。
- **M8-4b 原版**（startServerV2 hook + probe + RestartStartupSequence）：補丁新增 helpers 與守門邏輯未動到既有 pipeline struct 欄位與 stage hook 位置 ✅。
- **MAJ-4 補丁**（shutdown-imminent broadcast + notifyShutdownImminent）：不觸及 startup pipeline / server_control 的 startInternal / RestartStartupSequence 區塊 ✅。
- `grep` 未見 `<<<<<<<` / `>>>>>>>` 合併標記。

---

## K. 3 Major 症狀驗證

### M-1（重複 OS 通知）

症狀：冷啟動 stage 3 失敗 → `emitError` 發通知 A → startInternal error 分支再發通知 B。

修復後的流程：
1. `startServerV2` 失敗 → `pipeline.FailStage(3, err)` → `emitError` 發通知「第 3 階段失敗」+ setState(Error) + current = -1
2. startInternal 收 err → `HasFailedStage()` 回 true → `pipelineHandled = true` → **skip** setState/emit/sendCrashNotification
3. 只剩一個通知 ✅

**症狀消失確認**：Major M-1 修復路徑正確，唯一前提是「startServerV2 的所有失敗路徑都走 FailStage」— 已確認 L437-448（stage 2）、L554-566（stage 3）、stage 4 只在 timeout 仍 complete 不 fail，符合條件。

### M-2（重複開瀏覽器）

症狀：冷啟動成功 → runStartupStage5 的 openBrowser 呼叫 1 次 + startInternal R5-D3 的 openBrowser 呼叫 1 次 = 2 次。

修復後的流程：
1. 冷啟動 → startServerV2 成功 → startInternal 檢查 `IsInColdStart()`（此時 current ∈ 1..6）→ 回 true → **skip** R5-D3 openBrowser
2. startServerV2 返回後，app.startup 呼叫 `runStartupStage5` → openBrowser 1 次 ✅
3. RestartServer 路徑：pipeline.current==7 → IsInColdStart 回 false → R5-D3 正常 open ✅

**症狀消失確認**：冷啟動 openBrowser 只被呼叫一次。`TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce` 驗證 openCount==1。

### M-3（Retry test 重構）

症狀：test 手動複製 Step 1-5，改步驟順序抓不到。

修復後：直接呼叫 `a.RestartStartupSequence()` + callLog spy。若未來調整步驟順序（cancel 晚於 start），callLog 順序斷言會 fail ✅；若把 sentinel 清除改到 start 之後（Step 4 → Step 6 之後），`sentinelClearedBeforeStart` 斷言會 fail ✅。

**概念驗證**：test 能抓到「cancel 必須早於 start」與「sentinel 必須在 start 前清」兩個順序不變量，涵蓋 M-3 的核心意圖。

---

## L. 問題清單

### Critical
（無）

### Major
（無）

### Minor

| # | 檔案 | 行 | 問題 | 建議 |
|---|------|----|------|------|
| m-1 | `visiona-local/startup_pipeline_test.go` | L518-521 | 註解說「清理新啟動的 watcher goroutine」，但 `newPipelineTestApp` 的 `a.ctx == nil`，Step 5 的 watcher 分支不會啟動，pipelineCancelFn 在 Step 1 已被清為 nil。註解誤導但無功能影響 | 改註解為「若 ctx 存在則清理重建的 watcher（此 test 走 nil ctx 分支，為 safety）」 |
| m-2 | `visiona-local/server_control.go` | L167-194 | `pipelineHandled` 守門建立在「startServerV2 所有失敗路徑都走 FailStage」的隱性假設上，若未來新增一條不透過 FailStage return err 的路徑，M-1 症狀會復活 | 在 `startServerV2` 函式開頭加註解「所有 err return 前必須先 FailStage」；或改用 explicit flag（如 `a.ctrl.suppressFallbackError`）由 `emitError` 設，startInternal 讀後 clear |

### Suggestion

| # | 內容 |
|---|------|
| s-1 | `TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce` 嚴格來說驗證的是 runStartupStage5 本身只 open 一次（因為 restartStartFn 替換了 ctrl.Start，完全 bypass 了 startInternal 的 R5-D3 分支）。M-2 的守門邏輯（`IsInColdStart() == false` 才 open）是靠 `TestStartupPipeline_IsInColdStart` 單獨驗證的 pure boolean 邏輯。兩個 test 合起來足以推得正確性，但若希望更貼近「真實 M-2 症狀」，可另加一個 test 透過 `a.ctrl.startInternal` 直接呼叫（需要先在 ctrl 加 test-only hook 允許跳過 spawn），非必要 |

---

## M. 結論

**判定：✅ 通過**

補丁對 M8-4b 原版 Reviewer 提出的 3 Major 全部正確修復：

1. **M-1（重複通知）** — 新增 `HasFailedStage()` helper + `startInternal` error 分支守門，邏輯正確、lock 保護得當，regression test（`TestStartupPipeline_HasFailedStage`）覆蓋 4 case 全通過。
2. **M-2（重複開瀏覽器）** — 新增 `IsInColdStart()` helper + `startInternal` R5-D3 守門 + `openBrowser` 改 package-level var 以便 stub，regression test（`TestStartupPipeline_IsInColdStart` + `TestRestartStartupSequence_ColdStartOpenBrowser_OnlyOnce`）驗證完整。
3. **M-3（Retry test 重構）** — 新增 `restartStartFn` test hook + 重構 `TestStartupPipeline_RestartStartupSequence_StepsExecution` 為直接呼叫 method + callLog spy 驗證順序，抓得到 cancel↔start 與 sentinel↔start 的順序不變量。

**Stage 4 probe timeout** 從 5s 改為 2s，註解說明符合 TDD 原意。

**工程品質**：
- Build / Vet / Test / Race（-count=2）在 visiona-local 與 server 兩個 module 全綠
- 合併乾淨，無衝突標記
- 註解詳盡（每個 helper 都有中文註解解釋「為什麼」與「哪些場景走 fallback」）
- Lock 使用正確（`sync.Mutex` + `Lock/Unlock`，沒有 race condition）
- Test hook 透過 struct field 注入，不污染正式路徑（`restartStartFn` 預設 nil）

**不阻擋 M8-10**。Minor 2 項 + Suggestion 1 項可記為技術債，不強制當下處理。

**優點**：
- Helper 命名清楚（`HasFailedStage` / `IsInColdStart` 語義一眼就懂）
- 守門邏輯對 `pipeline == nil` fallback 處理完整，不會因單測環境或非冷啟動路徑被誤傷
- 4 個 regression test 覆蓋面完整：helper 本身 + 實際 RestartStartupSequence 流程 + openBrowser 呼叫次數
- 透過改 `openBrowser` 為 package-level var 解決測試 stub 問題，是 Go idiomatic 的做法
- `restartStartFn` test hook 設計乾淨：預設 nil → 走正式 `a.ctrl.Start()`，單測替換即可，不會意外漏到 production
- 2s probe timeout 的註解說明了「為什麼不是 5s」— 文件化設計決策