visionA/local-tool/.autoflow/05-implementation/review/m9-3-api-handler-ws-review-round2.md

# M9-3 Reviewer Round 2 — API handler + WebSocket（第 2 輪驗證）

- **審查日期**：2026-05-25
- **被審任務**：M9-3 第 2 輪修改（吸收第 1 輪 1 Major + 3 Minor + 5 Suggestion）
- **第 1 輪報告**：`.autoflow/05-implementation/review/m9-3-api-handler-ws-review.md`
- **本輪範圍**：delta 驗證（不重審第 1 輪）

## TL;DR

**通過。** Major 1（雙 JSON 鍵）採方案 A 修法乾淨、`firmwareVer` 鍵已從 derived struct 移除、JSON 輸出單一 SoT 是 `DeviceInfo.firmwareVersion`、`TestEnrichDevicesJSONOutput` + `TestEnrichDevicesJSONOutput_NilFwHandler` 兩個 test 鎖死不會再 regression。Minor 1-3 修法到位（ctx godoc + cache 不污染 + JSON 結構斷言）。Suggestion S-1 / S-3 / S-5 都修了；S-2 / S-4 backend 選擇不修分析正確、同意。**第 2 輪無新發現 Critical / Major / Minor**、只有 2 個極小 Suggestion（不阻擋）。**不阻擋 M9-4 frontend 開發**、**不需 backend 第 3 輪**。

## 第 1 輪 issue 修改驗證（逐項）

| Issue | 第 1 輪 | 第 2 輪修法摘要 | 驗證結果 |
|-------|--------|---------------|---------|
| Major 1 — JSON 雙鍵衝突 | derived `firmwareVer` 與 DeviceInfo `firmwareVersion` 同時輸出 | **方案 A**：刪 derived 的 `FirmwareVer` 欄位、frontend 直讀 `DeviceInfo.firmwareVersion` | ✅ 完全到位（見下方「Major 1 修法品質」） |
| Minor 1 — ctx.Background() 設計意圖 | 註解不夠清楚未來讀 code 會誤判為 bug | `firmware_handler.go:136-148` 補完整三段註解（為何不用 request ctx / service 內部 timeout / GracefulShutdown 路徑） | ✅ godoc 清楚解釋 |
| Minor 2 — bundledVersion cache poison | 失敗（檔不存在 / 讀錯 / 空檔）也 cache "unknown" 永遠不會 retry | 改成「只 cache success」、3 條失敗路徑（os.Open / io.ReadAll / TrimSpace=="" ）皆 return "unknown" **不**寫 cache | ✅ 邏輯清楚、`TestBundledVersionFor_CacheMissDoesNotPoisonRetry` + `TestBundledVersionFor_EmptyFileNotCached` 驗證 |
| Minor 3 — Test error 斷言過依賴 error message string | 3 處 case 用 `strings.Contains(error message)` | 改用 JSON 結構斷言 `resp.Error.Code` | ✅ DeviceNotFound（L310-326）/ UnsupportedChip（L347-357）/ ServiceErrors（L391-402）全 3 處改完 |
| Suggestion S-1 — isLegacyFirmware 對齊 bridge.py | 沒 cross-test 防 drift | 重寫規則（顯式列舉 + KDP1.x prefix + KDP2-9 forward-compat）+ `TestIsLegacyFirmware_BridgeParity` 21 個 case | ✅（見下方「S-1 規則重寫評估」） |
| Suggestion S-2 — forward done sleep grace | WS broadcast 完成 vs CleanupTask race | backend 分析 broadcast 同步、無 race、不修 | ✅ 分析正確（見下方「S-2 backend 不修評估」） |
| Suggestion S-3 — unknown error log | default branch 沒 log | `classifyServiceError` default branch 加 `log.Printf("[firmware_handler] classifyServiceError: unknown error type %T: %v", err, err)` | ✅ 含 type + value、診斷友善 |
| Suggestion S-4 — firmwareFeatureEnabled flag | nil fwHandler 與 disabled feature 無法區分 | YAGNI、留 follow-up | ✅ 同意（見下方「S-4 backend 不修評估」） |
| Suggestion S-5 — goroutine leak 驗證 | 沒測 forward goroutine 退出 | `TestForwardGoroutine_ExitsOnChannelClose` 驗 `CleanupTask` 被呼叫 1 次 == goroutine 退出 | ✅ 取代直接觀察 `runtime.NumGoroutine()` 的合理替代方案 |

**修復統計**：8/9 直接修了、1 個（S-4）backend 選不修並附理由。Reviewer 同意。

## Major 1 修法品質

### Before / After diff（concept）

```go
// Before（第 1 輪）：
type FirmwareDerivedFields struct {
    FirmwareVer            string `json:"firmwareVer"`             // ← 與 DeviceInfo.firmwareVersion 衝突
    FirmwareIsLegacy       bool   `json:"firmwareIsLegacy"`
    FirmwareCanUpgrade     bool   `json:"firmwareCanUpgrade"`
    BundledFirmwareVersion string `json:"bundledFirmwareVersion"`
}

// After（第 2 輪）：
type FirmwareDerivedFields struct {
    FirmwareIsLegacy       bool   `json:"firmwareIsLegacy"`
    FirmwareCanUpgrade     bool   `json:"firmwareCanUpgrade"`
    BundledFirmwareVersion string `json:"bundledFirmwareVersion"`
}
// → device list response 只有一個 firmware 字串鍵 = firmwareVersion（來自 DeviceInfo）
```

### 驗證點

| 驗證項 | 狀態 |
|--------|------|
| `firmware_handler.go:249-253` FirmwareDerivedFields struct 真的只剩 3 欄位 | ✅ |
| `DeriveFirmwareFields` return 不再含 firmwareVer | ✅ `firmware_handler.go:267-278` 確認只 set 3 欄位 |
| `device_handler.go:64-67` deviceWithFirmware 結構未變、但 embed 的 FirmwareDerivedFields 已縮減 | ✅ |
| `enrichDevices` nil fwHandler 分支 fallback 也不洩漏 firmwareVer | ✅ `device_handler.go:82-84` 只 set BundledFirmwareVersion |
| `POST /api/devices/scan`（ScanDevices）走相同 enrichDevices、一致性 | ✅ `device_handler.go:91-104` |
| `TestEnrichDevicesJSONOutput` JSON marshal 驗 `firmwareVersion:"KDP"` + 否定 `firmwareVer` | ✅ `firmware_handler_test.go:650-697` |
| `TestEnrichDevicesJSONOutput_NilFwHandler` fallback 路徑也驗雙鍵不出現 | ✅ `firmware_handler_test.go:701-726` |
| `TestDeriveFirmwareFields_NoFirmwareVerKey` 直接驗 FirmwareDerivedFields JSON 不含 firmwareVer | ✅ `firmware_handler_test.go:619-640` |
| `TestDeriveFirmwareFields_EmptyFirmwareDir` 補一個 grep firmwareVer 否定斷言 | ✅ `firmware_handler_test.go:609-613` |
| 註解 `device_handler.go:60-63` + `firmware_handler.go:243-248` 留下「為何不再有 firmwareVer」決策足跡 | ✅ 未來 reader 知道是刻意設計 |

### Frontend 影響評估

- M9-4 frontend 尚未上線消費此 endpoint → 0 regression 風險。
- Frontend 之後實作時直接用 `device.firmwareVersion`（既有命名、TypeScript 既有 type 應已含此欄位）+ 3 個衍生 boolean 欄位、API contract 乾淨。
- TDD §3.1 line 131 原本寫 `firmwareVer`、與實作 drift。建議 backend 在第 2 輪後**同步更新 TDD 文件**（把 `firmwareVer` 改為 `firmwareVersion`、註明改動原因）。此非 Critical、可由 architect 在 M9-4 / M9-5 任一時機追加。

### Round 1 Major 1 升級理由 vs Round 2 修法

第 1 輪報告（L112-115）的升級理由是「屬 API contract 對外可見的 schema bug、影響 M9-4」。Round 2 採方案 A 的修法**完全消除**這個問題：
- ✅ API contract 對外只有單一 firmware 字串鍵
- ✅ M9-4 不需要二選一決策（直用既有 `firmwareVersion`）
- ✅ 沒破壞既有 `/api/devices` API（DeviceInfo.FirmwareVer JSON tag 維持 `firmwareVersion`）
- ✅ 新增 3 個衍生欄位（isLegacy / canUpgrade / bundledFirmwareVersion）是純加法、不衝突

## S-1（isLegacyFirmware）規則重寫評估

### 規則邏輯（`firmware_handler.go:375-406`）

```
1. fw == ""              → false（Go 端 list 場景保守、與 bridge.py 差異已 godoc）
2. fw in {KDP, KDP1, USB BOOT, USB BOOT LOADER, LOADER, BOOTLOADER} → true
3. HasPrefix "KDP1." or "KDP1 "  → true
4. HasPrefix "KDP2".."KDP9"      → false（forward-compat 明示放行）
5. default                       → false（未知字串保守）
```

### TestIsLegacyFirmware_BridgeParity 真值表評估

| 群組 | case 數 | 評估 |
|------|--------|------|
| KDP1 legacy exact | 7（KDP / KDP1 / USB BOOT / USB Boot / USB BOOT LOADER / LOADER / BOOTLOADER）| ✅ case insensitive 也測 |
| KDP1.x 變體 | 3（KDP1.0 / KDP1.5 / KDP1 alpha）| ✅ 覆蓋 prefix space 與 dot 兩種 |
| KDP2-9 forward-compat | 6（KDP2 / KDP2.0 / KDP2-v2.2.0 / KDP3 / KDP3.1 / KDP9）| ✅ 含 plain prefix + version suffix |
| 未知字串 → 保守 | 3（NEF / K3 / v2.2.0）| ✅ |
| 空字串 Go 特例 | 1（""）| ✅ 含 godoc 註明 Go vs bridge.py 差異 |
| **合計** | **20** | ✅ |

### 「Go vs bridge.py 空字串差異」評估

第 1 輪 Suggestion 提的「跨端 drift 防護」核心目的是「將來 bridge.py 字串變動時、Go 端能立刻 fail test 提醒同步」。本輪實作的差異設計：

- bridge.py：空字串 = legacy（因為 USB Boot state 在 connect / upgrade 流程下不回 firmware 字串）
- Go list endpoint：空字串 = 非 legacy（因為 list 場景看到空字串 = device 還沒 connect 過、不該顯示「升級按鈕」）

**這個差異是正確的設計**（不是 bug），原因：
1. **語意分層**：bridge.py 的 `_fw_classify_legacy` 跑在 connect / upgrade 上下文、空字串確實代表「device 在 USB Boot 等 loader stage」、應視為 legacy；Go list endpoint 跑在「user 看 device 清單」上下文、空字串通常代表「driver 還沒讀過 firmware 字串」、不該誤導 user 點升級。
2. **不會造成功能漏洞**：真正會升級的 legacy device、一定是先 connect 過、`d.FirmwareVer` 會是 "KDP" 等非空字串、走 rule 2 / 3 判定為 legacy；空字串只發生在 device 剛 detect 但未 connect 的中間狀態。
3. **godoc 已明確標記**（`firmware_handler.go:382-385` + test L734-735）→ 未來 reader 不會誤認為 bug、也不會被 test 誤導去「修齊」。

**Reviewer 結論**：差異設計合理、godoc 完整、test 鎖定 invariant。**沒問題**。

### S-1 邊角顧慮（極小、列為 Suggestion）

- **潛在 edge case**：bridge.py 真值表也含「product_id == 0x0200 → legacy」一條（KL720 USB Boot product ID）。Go 端 test L737-738 明示「不檢查 product_id」、理由是 KL720 KDP legacy 由 chip type + upgrade 流程處理。**確認此設計：** Go list 場景下 KL720 device 的 `Type` 字串會是 `kneron_kl720`、`FirmwareVer` 若為 `"KDP"` 會被 rule 2 判 legacy、`SupportedUpgradeChip("KL720")` = true → CanUpgrade=true。流程閉合、product_id 在此 endpoint 沒有獨立角色。**OK**。

## S-2 backend 選擇不修評估

### Backend 第 2 輪分析

「forward 同步消費完才 return、WS broadcast 是同步呼叫、無 race」

### Reviewer 確認分析

逐行檢查 `forwardProgressToWS`（`firmware_handler.go:173-186`）：

```go
func (h *FirmwareHandler) forwardProgressToWS(deviceID string, progressCh <-chan firmware.FirmwareProgress) {
    room := "firmware:" + deviceID
    for ev := range progressCh {
        h.wsHub.BroadcastToRoom(room, firmwareProgressMessage{...})  // ← 同步呼叫
    }
    h.svc.CleanupTask(deviceID)  // ← for-range 退出後才呼叫
}
```

**Race 分析**：
1. Service close progressCh 時、最後一個 event 已經先 send 進 channel（`firmware/service.go` 的 `runUpgrade` 終態先 send done event → 再 close）
2. forward goroutine for-range 會把 channel 內所有剩餘 event 全部消耗完才退出
3. `BroadcastToRoom` 是**同步**呼叫（ws.Hub 的實作）→ 該方法 return 才表示已把 message hand off 給 ws connections（非同步 deliver 但同步 enqueue）
4. for-range 退出 → 表示「最後一個 event 的 enqueue 已完成」→ 此時呼 `CleanupTask` 移除 tracker entry

**race window 是否存在？**

| 場景 | 結果 |
|------|------|
| WS connections 還沒收到最後的 done event、但 `BroadcastToRoom` 已 return | 是 ws.Hub 內部的「enqueue 完成 vs network deliver 完成」 lag、與 CleanupTask 無關 |
| client 收到 done event 後立刻 GET /api/firmware/active-tasks | 此時 CleanupTask 已執行、看不到 task | **這才是 S-2 提的問題** |

**但是！** S-2 提的問題的根因是「client 用 GET /active-tasks 來確認 task 結束」、而**正確的 UX flow** 應該是「client 收到 done event 後直接信任、不再 poll /active-tasks」。`/active-tasks` 的目的是 graceful shutdown 偵測（TDD §8.6.2、Wails OnBeforeClose）、不是 task 結束確認。

**所以 backend 的分析是對的**：當前架構下不需要 sleep grace。如果 M9-4 frontend 設計時真的撞到 UX flick 問題（modal 還在「完成」動畫、就被 /active-tasks 告知 hasActive=false），那是 frontend 端的「等動畫播完才 refresh active-tasks」職責、不是 backend。

✅ **Reviewer 同意 backend 不修 S-2**。

## S-4 backend 選擇不修評估

### Backend 第 2 輪分析

YAGNI、留 follow-up。

### Reviewer 確認

S-4 提的場景：**fwHandler=nil（feature disabled / 沒帶 firmware bundle）vs fwHandler 存在但 device 不可升**、前端無法區分。

**現實場景檢查**：
- Production build：一定有 firmware bundle（firmware/<chip>/VERSION 包進 release）→ fwHandler 一定非 nil
- Dev mode：可能 fwHandler 是 nil、但 dev 不會關心 user UX 細節
- Test mode：明示 mock、不在乎

**結論**：production 路徑 fwHandler 一定非 nil、`firmwareFeatureEnabled` flag 確實沒有 immediate value。等真有「shipping product 想刻意關掉 firmware feature」場景（如 OEM custom build）再加。**YAGNI 同意**。

✅ **Reviewer 同意 backend 不修 S-4**。

## 第 2 輪新發現

### Critical
無。

### Major
無。

### Minor
無。

### Suggestion（極小、不阻擋）

#### S2-1 — `bundledVersionFor` 失敗不 cache 的 thundering herd 顧慮（極低風險）

**位置**：`firmware_handler.go:288-320`

**情境**：production 環境若有 N 個 user 同時 GET /api/devices、每個都 call enrichDevices → 對所有 device chip 都 call bundledVersionFor → 若 VERSION 檔在那個時間點剛好不存在（不合理、production 應該一定有）、N 個請求都會走 `os.Open` + `io.ReadAll`、不命中 cache。

**評估**：
- production VERSION 檔不存在 = bug、屬於部署錯誤、不該設計 cache 容忍
- dev / CI first build 場景下「沒 VERSION 檔」是 transient、第一次 ready 後立刻成功 cache、不會持續打 disk
- disk stat / read 64 bytes 的 cost ~µs 級、即使一秒 100 個請求同時打、cost 仍 negligible
- 加 negative cache（如 cache "unknown" 30s TTL）會帶來時序複雜度（需要 timestamp、需要過期判斷）、value 不明顯

**Reviewer 結論**：**不阻擋、不修**。當前設計（只 cache success）的可預測性 > 加 TTL 的最佳化、是正確的取捨。如果未來 production 真的觀察到 disk I/O 熱點再加 TTL。

#### S2-2 — TestIsLegacyFirmware_BridgeParity test 名稱命名

**位置**：`firmware_handler_test.go:770-778`

**現況**：`t.Run(tc.fw+"_"+tc.note, ...)` 用 firmware 字串 + note 拼出 subtest name。Go test name 對空格 / 特殊字元的處理會自動 escape（`USB BOOT` → `USB_BOOT`）、不會 break、但 test 輸出可讀性稍差（一行 subtest 名稱很長）。

**評估**：純美學、不影響功能 / 覆蓋率。

**建議**（純可選）：將來若想優化、可改為 `t.Run("case_NN_"+shortLabel, ...)`、或拆 cases 為多個 named t.Run 群組（KDP1Legacy / KDP1xVariant / KDP2to9 / Unknown / Empty）。M9-3 階段不需要。

#### S2-3 — TestForwardGoroutine_ExitsOnChannelClose 的「goroutine 沒 leak」是間接驗證

**位置**：`firmware_handler_test.go:850-882`

**現況**：透過 `cleanupCalls == 1` 證明 forward goroutine 退出。**這個證明是充分的**（goroutine 退出後才呼 CleanupTask、且只呼一次）。

**評估**：第 1 輪 S-5 建議「`runtime.NumGoroutine()` snapshot 對比」是更直接、但本實作的「觀察副作用」也達成相同目的、且不依賴 runtime 內部行為（有時 NumGoroutine 不穩定、會被 test framework / GC goroutine 干擾）。

**Reviewer 結論**：當前實作策略**比建議的更穩定**。✅ 不修。

## TDD 文件 follow-up

`docs/autoflow/04-architecture/v2/firmware-management.md` §3.1 line 131 原本寫 `firmwareVer`（衍生欄位之一）。Backend 第 2 輪刪掉這個欄位、改用既有 `firmwareVersion`。**建議在 M9-4 或 M9-5 任一時機**請 architect 同步更新 TDD：

```diff
- 衍生欄位：firmwareVer / firmwareIsLegacy / firmwareCanUpgrade / bundledFirmwareVersion
+ 衍生欄位：firmwareIsLegacy / firmwareCanUpgrade / bundledFirmwareVersion
+ （注意：firmware 字串值用 DeviceInfo 既有 `firmwareVersion` 欄位、不重複輸出）
```

非 blocker、不影響 M9-4 frontend 開發（frontend 用實際 API contract 而非 TDD 文字）。

## 5 軸評估（Round 2 delta）

| 軸 | Round 1 結果 | Round 2 變化 |
|----|-------------|-------------|
| Correctness | ✅ Pass | ✅ Pass — 修法不引入新 edge case、isLegacy 規則重寫覆蓋齊全、bundledVersion 失敗不 cache 邏輯正確 |
| Readability | ✅ Pass | ✅ Pass — godoc 改善（ctx.Background / isLegacy / FirmwareDerivedFields 都有「為何這樣寫」決策足跡）、可讀性比 Round 1 更高 |
| Architecture | ✅ Pass | ✅ Pass — 沒動結構、只調整 struct 欄位與 helper 內部邏輯 |
| Security | ✅ Pass | ✅ Pass — 沒新增 endpoint / 沒動 auth、無新攻擊面 |
| Performance | ✅ Pass | ✅ Pass — bundledVersion 失敗不 cache 的 I/O 顧慮已評估為可接受（見 S2-1） |
| Test | ✅ Pass | ✅ Pass — 新增 6 個 test（Round 1 26 個 → Round 2 32+ 個）、Major 1 + S-1 + S-5 + M-3 + M-4 都有專屬覆蓋 |

## 結論

| 維度 | 狀態 |
|------|------|
| 第 1 輪 issue 處理 | ✅ 8/9 修了、1 個（S-4）合理 defer |
| 第 2 輪新發現 | 0 Critical / 0 Major / 0 Minor / 3 極小 Suggestion（純評估、不阻擋） |
| 是否阻擋 M9-4 | ❌ **不阻擋** — frontend 可以開工 |
| 是否需 backend 第 3 輪 | ❌ **不需要** — 當前狀態已可進 M9-4 |
| 是否升 security agent | ❌ 不需要（與 Round 1 結論同） |

**整體：✅ 通過。**

### 優點（明確讚美）

1. **Major 1 採方案 A 修法最乾淨**：不破壞既有 `firmwareVersion` API contract、derived struct 縮減 1 欄、frontend 不需學新 schema、test 同時補了「在 enrichDevices JSON 輸出層」與「在 FirmwareDerivedFields 自身層」兩層 guard、未來想 regression 也辦不到。
2. **isLegacyFirmware 重寫質量**：規則從第 1 輪的「KDP 字串簡單比對」升級為「顯式列舉 + KDP1.x prefix + KDP2-9 forward-compat 放行 + default 保守」、Go vs bridge.py 差異點（空字串）有 godoc 註記、20-case 真值表鎖定 invariant。明顯有 cross-端思考（python ↔ go drift 防護）、不只是「pass test 就好」的工程心態。
3. **bundledVersion cache poison 修法**：3 條失敗路徑（os.Open / io.ReadAll / TrimSpace == ""）全部用「不寫 cache」處理、test 兩種 fail mode（無檔 + 空檔）都覆蓋、未來 reader 一看就懂為何分支多。比起加 TTL 的方案、可預測性更高、value/complexity 比優異。
4. **godoc 「為何這樣寫」的決策足跡**：ctx.Background 三段註解、Major 1 修改決策、isLegacy 跨端差異說明 — 都不只是「描述 what」而是「解釋 why」。M9-5 / M9-11 future maintainer 不會誤改。

## Verification 自評

### A 層（每個 review 必做）
- [x] R-A1：5 軸 + 測試軸全跑過、每軸實質判斷 ≥ 20 字（見上方「5 軸評估」表）
- [x] R-A2：第 1 輪 issue 修法逐項表 + Major 1 修法品質表（含驗證點 + frontend 影響評估）、2 張 cross-doc 表完整
- [x] R-A3：本輪 Critical / Major / Minor 都 0、Suggestion 3 個附 line number + 評估
- [x] R-A4：寫明優點 ≥ 1 條（實際 4 條）
- [x] R-A5：明示「本輪無不確定項」
- [x] R-A6：§12.2 通用 6 條：no silent failures（cache miss 明確 return + 不 cache）/ no dead code / no hardcoded secrets / no unsafe HTML/SQL / doc 同步（TDD §3.1 follow-up 已列出、待 architect 同步）/ 被審 commit clean（屬被審 agent 責任、未深查）

### B 層（milestone）
- 暫緩 B 層 verification。
- 原因：本輪是 Round 2 delta 驗證、檔案範圍與 Round 1 完全重疊（3 個檔案）、不存在新增跨檔案不一致風險。
- 預計補做時機：M9-5 三平台驗收時統一跑跨檔案 + 跨端 parity 驗證。
- 風險評估：低（M9-4 frontend 是 schema 消費端、會自然 catch backend schema 不一致）。

### C 層（PR / merge）
- 不適用（M9-3 內部 milestone review、非 PR 階段）。