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

# Reviewer 審查 M8-4b 啟動階段管線（2026-04-15）

## 摘要

- 審查對象：M8-4b（R5-E 6 階段啟動 pipeline + watcher + sentinel file + RestartStartupSequence）
- 審查檔案：`visiona-local/startup_pipeline.go`（394 行）、`startup_pipeline_test.go`（457 行）、`server/internal/api/ws/hub.go`（sentinel 機制）、`hub_sentinel_test.go`（3 tests）、`visiona-local/app.go`（startup / shutdown / runStartupStage5）、`visiona-local/server_control.go`（stage hooks / probeDeviceListAndComplete / RestartStartupSequence / ForceKill）、`server/main.go`（wiring）
- 總結論：**⚠️ 需小改** — 核心實作對齊 TDD v2/startup-pipeline.md，Build/Vet/Test/Race 全過，功能面沒有阻擋 M8-5 / M8-9 的問題；但發現 **2 個 Major UX 問題**（重複 OS 通知 + 重複開瀏覽器），建議在 M8-4b 收尾前順手修。
- 問題統計：Critical 0 / Major 2 / Minor 5 / Suggestion 3
- 阻擋後續 milestone 嗎：**否**。M8-5（前端控制台 UI）可直接使用現在的 event schema 與 `RestartStartupSequence` binding；M8-9（boot-id + 重連）不觸及 pipeline 內部，可平行推進。兩個 Major 屬 UX 層面，在 M8-10 end-to-end 驗收前修掉即可。

---

## A. StartupPipeline struct

| TDD §4 要求 | 實作位置 | 狀態 |
|------|---------|------|
| 6 階段常量 / soft 20s / hard 60s / tick 1s | `startup_pipeline.go` L39-44 | ✅ |
| Event schema（`StartupProgressEvent` / `StageTimeoutEvent` / `ErrorEvent`）| L54-75 | ✅ 欄位命名、json tag 與 §1 一致 |
| Status 枚舉含 pending/running/completed/failed/skipped | L60, L82 | ✅ 和 §1.1 v2.1 新增 `skipped` 對齊 |
| 1-indexed stages 陣列 + `current` sentinel 值 | L99-101 | ✅ 0/1-6/7/-1 語義註解清楚 |
| `CompleteStage` 順序保護 + `current != stage` 忽略 | L138-163 | ✅ 符合 §4「重複呼叫或順序錯誤，忽略」 |
| `SkipStage` → 進下一階段 | L168-192 | ✅ 與 `CompleteStage` 對稱 |
| `FailStage` → `emitError` + `stopWatcher` | L200-213 | ✅ |
| `markReady` → emit `startup:ready` + `stopWatcher` | L216-226 | ✅ 成功路徑走同步 emit（註解有解釋） |
| `emitProgress` 用 goroutine 非阻塞 | L230-251 | ✅ 符合 §4 關鍵設計 3 |
| `emitError` 同步 `setState(Error)` + 非阻塞 OS 通知 | L255-277 | ✅ |

**符合度 100%**。

---

## B. Watcher goroutine

| 行為 | 位置 | 狀態 |
|------|------|------|
| 每秒 tick + `ctx.Done` 退出 | L291-297 | ✅ |
| `current` 不在 1-6 範圍時 return | L300-303 | ✅ |
| 階段 6 每 tick 檢查 sentinel file | L313-318 | ✅ |
| `skipped` status / 階段 6 + AutoOpenBrowser=false 跳過 timeout | L321-327 | ✅ 雙條件都檢查 |
| Hard timeout 直接展開 mark failed + `emitError(total-timeout)` | L329-341 | ✅ cause 分流正確；不走 `FailStage` 以便 cause 設為 `total-timeout` |
| `softTimeoutEmitted` flag 每階段最多 emit 一次 | L348-362 | ✅ |
| `skipTimeout` continue 時不檢查 soft timeout | L343-345 | ✅ |

**Minor B-1**：`watcher` 讀取 `p.app.prefs.AutoOpenBrowser`（L325）未持 `a.mu`，但 `SetPreferences`（`server_control.go` L1062-1064）寫 `a.prefs` 時持 `a.mu`。因為 `Preferences` 是 struct value（含 `bool` + `string`），同時讀寫觸發 data race。實測環境罕見（使用者在 Starting state 改 pref），`go test -race` 沒觸發。建議 watcher 的 pref 讀取改走 getter 或先 snapshot。

---

## C. Stage hooks 插入位置

| Stage | 位置 | 狀態 |
|-------|------|------|
| 1 Wails 控制台 | `app.go` L224 `CompleteStage(1)`（在 `seedUserDataDir` 之後、`ctrl.Start` 之前）| ✅ 對齊 §3 |
| 2 Python runtime | `server_control.go` L437-448 `FailStage(2)` / `CompleteStage(2)` 夾 `ensurePythonRuntime` | ✅ |
| 3 Server spawn | L554-566 `FailStage(3)` / `CompleteStage(3)` 夾 `waitHealthy` | ✅ |
| 4 Device probe | L571-576 呼叫 `probeDeviceListAndComplete(port)` → HTTP GET `/api/devices` 5s timeout → 任何 response/err 都 `CompleteStage(4)` | ✅ 行為與 §3 描述一致（「秒回算完成」），但 5s timeout 過於保守 — 見 §I-2 |
| 5 Open browser | `app.go` L243-267 `runStartupStage5()` helper：`AutoOpenBrowser=false` → `SkipStage(5)`，`true` → `openBrowser(url)` + `CompleteStage(5)` | ⚠️ **見 Major C-1** |
| 6 WebSocket ready | 由 watcher poll sentinel file 觸發 | ✅ |

**Major C-1：瀏覽器會被開兩次**
`runStartupStage5`（app.go L262）呼叫 `openBrowser(url)`，但 `startInternal`（server_control.go L188-194）的 R5-D3 邏輯在 Start 成功後也 `openBrowser(url)`。冷啟動路徑會執行兩次 `openBrowser`：
- macOS：`open http://...` 同 URL 兩次通常聚合到同一 tab，但取決於瀏覽器
- Linux：`xdg-open` 可能開兩個 tab
- Windows：`start` 行為不一致

建議的修法（二擇一）：
1. `startInternal` 在 `a.startupPipeline != nil && a.startupPipeline.current >= 0 && a.startupPipeline.current <= startupTotalStages`（冷啟動中）時跳過自己的 `openBrowser`，由 `runStartupStage5` 負責；`RestartServer` 情境下 pipeline 已 ready（`current==7`），R5-D3 仍執行。
2. 反過來：`runStartupStage5` 只負責 `CompleteStage(5)`，實際 open 由 startInternal 處理 — 但這樣做跟 TDD §3 階段 5 的語義（「Open browser」＝呼叫 OpenInBrowser 返回）不符。
推薦方案 1。

---

## D. Sentinel file 機制

| 項目 | 位置 | 狀態 |
|------|------|------|
| `Hub.writeStartupSentinel` 用 `sync.Once` | `hub.go` L79-98 | ✅ |
| Register channel 第一次收到 Subscription 觸發 | L112 | ✅ 在 `h.mu.Unlock()` 之後呼叫（避免重入） |
| 檔案內容 `bootId=... ts=...` | L95 | ✅ 符合 §3 |
| 清檔時機：startup 早期（`app.go` L175）+ shutdown（L299）+ RestartStartupSequence Step 4（L844）| | ✅ 三處都清，符合 §3 的 best-effort 策略 |
| `Hub.SetStartupSentinel` 從 `server/main.go` L133 注入 `cfg.DataDir`（L101）| | ✅ |
| DataDir 空值時 disable | `hub.go` L85-87 | ✅ 有單測驗證（`DisabledWhenDataDirEmpty`）|

**Minor D-1**：`writeStartupSentinel` 取 `h.mu.RLock()` 保護 `sentinelDataDir`，但 `SetStartupSentinel`（L68-72）用 Write lock — 這個 lock 保護與 `rooms` map 的 lock 共用。功能上正確但概念上有點混搭（同一把 lock 同時保護「訂閱狀態」與「初始化一次的 dataDir」），未來可用獨立 sync.Once 或 atomic.Pointer 分離。

---

## E. RestartStartupSequence 5 步驟

| Step | 位置 | 狀態 |
|------|------|------|
| 1. cancel watcher goroutine | `server_control.go` L832-835 `a.pipelineCancelFn()` + 清 nil | ✅ |
| 2. ForceKill server subprocess | L838 `ctrl.ForceKill()`，該 method 在 L273-291 已實作（cancel watcher → clear proc → `proc.forceKill()` → `setState(Stopped)`）| ✅ |
| 3. Reset state machine to Stopped | L841 `setState(Stopped, "")` | ✅ 雖與 Step 2 結尾重複，但 defensive 沒壞處 |
| 4. Clean sentinel file | L844 `removeSentinelFile(a.dataDir)` | ✅ 符合 §3 明確要求「critical — 否則階段 6 會誤判瞬間完成」 |
| 5. 重建 `StartupPipeline` + stage 1 直接 complete + watcher + stage 2 running + `ctrl.Start()` + `runStartupStage5()` | L847-887 | ✅ 符合 §8.1 |
| Stage 1 不重跑 | L851-858 直接寫 `stages[1].status = "completed"` + emitProgress | ✅ |
| Binding 暴露 | Wails binding 是整個 App（`main.go` L35-37），所有 exported method 自動暴露 | ✅ `RestartStartupSequence() error` 符合前端呼叫介面 |

**Minor E-1**：Step 5 手動操作 `a.startupPipeline.mu` 和 internal fields（`stages[1].status = "completed"` 等），繞過了 struct 的封裝 API。未來改 `stageState` 欄位或 status 語義時，這段極容易 drift。建議把這段封裝為 `(*StartupPipeline).forceCompleteStage1And2Running(time.Time)` 或類似方法，把操作內部狀態的邏輯收進 pipeline 自己的 file。

**Minor E-2**：Step 5 L865 設 `a.startupPipeline.watcherCancel = cancel` 同一個 cancel 也存在 `a.pipelineCancelFn`。`FailStage → stopWatcher → watcherCancel()` 會 cancel，但 `a.pipelineCancelFn` 不會被 nil-out → 下次 Retry 執行 Step 1 會呼叫已 cancelled 的 func（`context.CancelFunc` 文件保證 idempotent，OK），然後清 nil。功能正確，但語義上「cancel 執行後清 nil」一致性不足。可接受。

**Minor E-3**：`RestartStartupSequence` 在 `a.ctx == nil` 情境下（單測走這條）不啟動 watcher，正常執行環境下 `a.ctx` 一定有值，但如果未來 ctx 時序改變，可能踩到。建議在 `a.ctx == nil` 時 return error 而非 silent skip watcher。

---

## F. Stage 5/6 skip 邏輯

| 規則 | 驗證 |
|------|------|
| Linux + `AutoOpenBrowser=false` → Stage 5 skipped | ✅ `preferences.go` L42 `DefaultPreferences()` 依 GOOS 把 Linux 設 false；`runStartupStage5` L247-250 呼叫 `SkipStage(5)` |
| `AutoOpenBrowser=false` → Stage 6 不 trigger timeout | ✅ `startup_pipeline.go` L325-327 skipTimeout 條件覆蓋 hard + soft |
| `skipped` 狀態 emit progress event | ✅ `SkipStage` L178 呼叫 `emitProgress(stage)`，status 已是 `"skipped"` |
| Watcher 在 stage 6 + AutoOpenBrowser=false 時不誤判 hard timeout | ✅ 有單測 `TestStartupPipeline_Watcher_SkippedStageNoTimeout`（set 70s sinceTotal 不觸發） |

一個**設計問題**（非 bug）：`AutoOpenBrowser=false` + 使用者永遠不手動開瀏覽器時，pipeline 永遠停在 stage 6 running → Startup panel 永遠不淡出。TDD §7 驗收條件 case 7 只說「階段 5 立即 complete」，沒說 panel 該什麼時候淡出。M8-5 前端設計需要補一個「AutoOpenBrowser=false 時直接把 panel 淡出 + 顯示『伺服器已就緒，請點 Open in Browser』」的流程。**這不是 M8-4b 的 bug，是 M8-5 要處理的 UX。** 建議 M8-5 Agent 啟動前提醒。

---

## G. 與 M8-4 補丁合併狀況

兩個並行改動：
- **M8-4 補丁**碰 `Stop()` / `ForceKill()` / `handleWatchFailure()` / `logPump()`（失敗與關閉路徑）
- **M8-4b** 碰 `startServerV2` 中間 hook 點 + 新增 `probeDeviceListAndComplete` + `RestartStartupSequence`（成功路徑）

合併狀況：
- `ForceKill`（L273-291）的 M8-4 修復（MAJ-1：`cancelWatcher` 先 cancel 避免 30s 後翻 Error）仍存在並被 `RestartStartupSequence` Step 2 正確利用。✅
- `handleWatchFailure`（L305-337）的 MAJ-2 修復（持 `txMu` + 檢查 state != Running 則 return）不被 M8-4b 直接依賴，但 `RestartStartupSequence` 的 ForceKill 路徑依賴「watcher 先被 cancel」的順序，兩個修復方向一致。✅
- `startServerV2` 沒被 M8-4 碰，M8-4b 只新增 pipeline hook，不破壞現有流程。✅

`cd visiona-local && go build .` → PASS
`cd visiona-local && go vet ./...` → PASS
`cd visiona-local && go test ./...` → PASS（7.263s）
`cd visiona-local && go test -race -count=2 ./...` → PASS（15.122s）
`cd server && go build ./...` → PASS
`cd server && go vet ./...` → PASS
`cd server && go test ./...` → PASS
`cd server && go test -race ./...` → PASS

---

## H. Build / Test / Race detector

全部 PASS（見 §G 尾）。

---

## I. Agent 觀察評估

### I-1 重複 OS notification — **Major I-1，真的會發生**

流程：
1. `startServerV2` 失敗（例如 stage 3 `waitHealthy`）→ L559 `pipeline.FailStage(3, err)`
2. `FailStage` → `emitError(3, err, "stage-failure")` → `startup_pipeline.go` L268-276：
   - `c.app.ctrl.setState(ServerStateError, err.Error())`
   - `go sendCrashNotification("visionA Local — 啟動失敗", "第 3 階段失敗：...")` ← **通知 1**
3. `startServerV2` return err → `startInternal` L166 收到 err
4. `startInternal` L168-180：
   - 再次 `setState(Error, err.Error())`
   - emit `server:error` event
   - `go sendCrashNotification("visionA Local — Server 啟動失敗", "請打開 visionA Local 查看錯誤詳情或按 Restart 重試。")` ← **通知 2**

兩則通知 title 和 body **都不同**，使用者會以為是兩個獨立錯誤；`setState(Error)` 被呼叫兩次（值相同、前端收到重複 `server:state-change` event，但 payload 一樣不致命）。

**建議修法**：在 `startInternal` L168-180 的 error 分支加守門：
```go
// 若 pipeline 已 FailStage 過，不重複發通知與 setState（pipeline 已處理）
if c.app.startupPipeline == nil || c.app.startupPipeline.current != -1 {
    c.setState(ServerStateError, err.Error())
    if c.app.ctx != nil {
        wailsRuntime.EventsEmit(c.app.ctx, "server:error", ...)
    }
    go sendCrashNotification(...)
}
```
或 `emitError` 設一個 `ctrl.suppressNextErrorNotification` flag，`startInternal` 收到 err 時若 flag 為 true 則 clear 並 skip 通知。後者耦合小。**阻擋 UX 可接受度，建議本次 M8-4b 一併修。**

### I-2 Stage 4 probe 5s timeout — **Minor**

TDD §3 原文「GET /api/devices 第一次收到 response（無論是否有硬體，秒回即算完成）」。實作用 `http.Client{Timeout: 5 * time.Second}` 並把 timeout err 也視為完成。

評估：
- 實測情境：`/api/devices` handler 讀內部 registry，ms 級就回，5s 幾乎不會用到
- 極端情境：deviceMgr Start 時非同步 USB 掃描若 block 主 goroutine（不太可能），handler 可能慢 >1s
- 5s 對 UX 來說：若硬體 driver 卡住而 health check 已 200，使用者會額外等 5s 看到 stage 4 才 complete。但因為 stage 4 timeout 也算完成，不 fail，只是卡視覺進度
- 對比 TDD「秒回」原意：5s 保守

**建議**：`Timeout: 2 * time.Second`。2s 足以涵蓋正常業務 latency 且符合 TDD 原意。Minor 優化，不阻擋。

---

## J. 測試品質

14 個 pipeline test + 3 個 hub sentinel test 覆蓋：

| 項目 | 測試 | 狀態 |
|------|------|------|
| CompleteStage 正常推進 | `CompleteStage_AdvancesToNext` | ✅ |
| CompleteStage 順序錯誤忽略 | `OutOfOrder_Ignored` | ✅ |
| CompleteStage 最後階段 → markReady | `LastStageMarksReady` | ✅ |
| SkipStage | `SkipStage_AdvancesAndMarksSkipped` | ✅ |
| FailStage | `FailStage_StopsPipeline` | ✅ |
| Soft timeout（mock 時間）| `Watcher_SoftTimeout` | ✅ 用 `startedAt = now.Add(-25s)` 加速 |
| Hard timeout（mock 時間）| `Watcher_HardTimeout` | ✅ 不是真的跑 60s |
| AutoOpenBrowser=false + stage 6 | `Watcher_SkippedStageNoTimeout` | ✅ |
| skipped status bypass | `Watcher_SkippedStatusBypassesTimeout` | ✅ |
| Sentinel file 偵測 | `CheckSentinelFile` / `Stage6CompletesOnSentinel` | ✅ |
| removeSentinelFile | `RemoveSentinelFile` | ✅ 驗證 idempotent + 空 dataDir |
| stopWatcher idempotent | `StopWatcher_Idempotent` | ✅ |
| Hub sentinel | 3 test（first register / only once / disabled） | ✅ |

**Major J-1**：`TestStartupPipeline_RestartStartupSequence_StepsExecution`（L386-439）**沒有呼叫 `RestartStartupSequence` method 本身**，而是把 Step 1-5 的邏輯**手動複製一遍**再驗證 side effect。這個測試保護性很弱：未來改 `RestartStartupSequence` 內部順序（例如把 Step 3 `setState(Stopped)` 拿掉 / 改順序），這個測試還是會過。建議重構為：把 Step 6 `ctrl.Start()` 的部分透過測試 double 攔截（例如改測 `app.ctrl.Start` 前的狀態 snapshot），直接呼叫 `a.RestartStartupSequence()` 驗證 Step 1-5 真的執行。

**Minor J-1**：沒有測試 race 情境「RestartStartupSequence 執行期間使用者再按 Retry」。雖然 `pipelineCancelFn()` 是 idempotent、`NewStartupPipeline` 是新 instance 不會 double free，但沒 test 保護。建議加一個 `TestRestartStartupSequence_Concurrent` 驗證連續呼叫兩次安全。

**Minor J-2**：沒有驗證 `emitProgress` 的 goroutine 路徑 — 因為測試環境 `a.ctx == nil` 讓 emit 走 short-circuit，只能間接驗證內部 state。可接受（Wails runtime 在單測中無法 mock）。

---

## K. 問題清單

### Critical

（無）

### Major

| # | 檔案 | 行 | 問題 | 建議修法 |
|---|------|----|------|---------|
| M-1 | `visiona-local/startup_pipeline.go` + `server_control.go` | `emitError` L272-276 + `startInternal` L168-179 | 啟動失敗時 OS 通知發兩次、標題內文不同，使用者誤以為是兩個獨立錯誤 | `startInternal` error 分支檢查 `startupPipeline.current == -1`（代表 pipeline 已 FailStage）時 skip `setState/emit/sendCrashNotification`；或在 `emitError` 設 suppress flag 讓後續路徑 skip |
| M-2 | `visiona-local/app.go` + `server_control.go` | `runStartupStage5` L262 + `startInternal` L188-194 | 冷啟動時 `openBrowser` 被呼叫兩次（R5-D3 R5-E stage 5 hook 重複） | `startInternal` 的 R5-D3 分支檢查「pipeline 是否在冷啟動中」（`current >= 1 && current <= 6`），若是則 skip，由 `runStartupStage5` 統一負責 |
| M-3 | `visiona-local/startup_pipeline_test.go` | L386-439 | `TestStartupPipeline_RestartStartupSequence_StepsExecution` 手動複製實作邏輯，未呼叫 method 本身，保護性弱 | 重構為直接呼叫 `a.RestartStartupSequence()`；把 `ctrl.Start()` 的 spawn 用環境旗標 skip（或 stub）使測試可執行 |

### Minor

| # | 檔案 | 行 | 問題 | 建議 |
|---|------|----|------|------|
| m-1 | `startup_pipeline.go` | L325 | watcher 讀 `a.prefs.AutoOpenBrowser` 未持 `a.mu`，與 `SetPreferences` 有潛在 race | 增加 getter `(a *App) preferencesSnapshot() Preferences`（內部持 `a.mu`），watcher 改呼叫 getter |
| m-2 | `server_control.go` | L847-887 `RestartStartupSequence` | 手動操作 pipeline internal fields（`stages[1].status = "completed"` 等），繞過封裝 | 封裝為 `(*StartupPipeline).RestartFromStage2(ctx, cancelStorer)` method |
| m-3 | `server_control.go` | L596-611 `probeDeviceListAndComplete` | HTTP timeout 5s 比 TDD「秒回」保守 | 改 `Timeout: 2 * time.Second` |
| m-4 | `server_control.go` | L862-868 `RestartStartupSequence` | `a.ctx == nil` 時 silent skip watcher，實際執行環境不會觸發但語義不清 | `a.ctx == nil` 時 return error `"app context not ready"` |
| m-5 | `startup_pipeline_test.go` | — | 沒有驗證 `RestartStartupSequence` 連續呼叫（race）| 加 `TestRestartStartupSequence_Concurrent`：`wg.Add(2); go restart(); go restart(); wg.Wait()` 驗證不 panic + 單一 pipeline instance 存活 |

### Suggestion

| # | 內容 |
|---|------|
| s-1 | `hub.go` `sentinelDataDir` 和 `sentinelOnce` 共用 `Hub.mu`，概念上混搭。可改用獨立 `sync.Once` 或 `atomic.Value` 分離初始化狀態與訂閱狀態 |
| s-2 | `watcher` 的 `skipTimeout` 判斷邏輯可抽成 helper `(p *StartupPipeline) shouldSkipTimeout(stage int, status string) bool`，提升可讀性 |
| s-3 | 失敗時的 OS 通知可考慮改成 in-app modal + OS 通知**擇一**（OS 通知常被使用者忽略），搭配 Wails 原生 dialog 效果更好 — 但這超出 M8-4b 範圍，屬於 M8-5 / M8-10 UX 階段決定 |

---

## L. 結論

**判定：⚠️ 需小改**

M8-4b 的核心工程實作（StartupPipeline struct / 6 階段 hook / watcher / sentinel file / RestartStartupSequence）完整對齊 TDD v2/startup-pipeline.md v2.1，Build/Vet/Test/Race 4 個平台全通過，合併 M8-4 補丁乾淨無衝突。

**阻擋 M8-5 / M8-9 嗎：否。** 目前的 event schema、binding 與 sentinel 機制已經足夠 M8-5（Wails 控制台 UI 訂閱事件）與 M8-9（boot-id 重連）直接開工。

**建議的後續動作**：
1. **M8-4b Agent 再跑一輪**，修 Major M-1（重複通知）+ M-2（重複開瀏覽器）+ M-3（Retry test 重構）。這三項都是工時小（< 30 分鐘）但影響使用者第一眼觀感的 UX 問題，在 M8-10 前必修。
2. **Orchestrator 交棒 M8-5 時**提醒 Frontend Agent：AutoOpenBrowser=false + stage 6 永遠 running 的情境下，startup panel 該如何淡出 / 顯示「請手動開瀏覽器」的 CTA。這個 UX 決定不屬於 M8-4b 範疇，但要在 M8-5 啟動前搞清楚。
3. Minor 5 項 + Suggestion 3 項可記為技術債，不強制 M8-4b 處理。

**優點**：
- TDD §4 結構完整重現，status 列舉 / 欄位 / emit 策略全部符合
- `skipped` 狀態在 watcher + CompleteStage/SkipStage 的處理一致，邊界條件清楚
- Test 用 mock 時間（`startedAt.Add(-Xs)`）加速 soft/hard timeout 驗證，沒有真跑 60 秒
- Sentinel file 三個清檔時機（startup 早期 / shutdown / Retry）覆蓋完整
- 與 M8-4 補丁（ForceKill / handleWatchFailure 的 MAJ-1/MAJ-2 修復）合併乾淨，`RestartStartupSequence` 正確利用了 ForceKill 的 cancel watcher 修復
- 測試覆蓋面積大（17 個 test），邊界條件完整