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）

// 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）：

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 提的問題的根因是「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//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：

- 衍生欄位：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 必做）

• R-A1：5 軸 + 測試軸全跑過、每軸實質判斷 ≥ 20 字（見上方「5 軸評估」表）
• R-A2：第 1 輪 issue 修法逐項表 + Major 1 修法品質表（含驗證點 + frontend 影響評估）、2 張 cross-doc 表完整
• R-A3：本輪 Critical / Major / Minor 都 0、Suggestion 3 個附 line number + 評估
• R-A4：寫明優點 ≥ 1 條（實際 4 條）
• R-A5：明示「本輪無不確定項」
• 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 階段）。