visionA/local-tool/.autoflow/05-implementation/reviews/M8-4b-patch-review.md

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」— 文件化設計決策