visionA/local-tool/.autoflow/05-implementation/review/m9-3-api-handler-ws-review.md
jim800121chen 5e281ed449 feat(local-tool): M9-3 — firmware API handlers + WebSocket progress room
A 階段第三個 milestone、暴露 firmware service 給 Frontend / Wails control panel。

New / modified:
- server/internal/api/handlers/firmware_handler.go: 新檔 465 行(upgrade + active-tasks endpoint + WS broadcast goroutine)
- server/internal/api/handlers/firmware_handler_test.go: 新檔 938 行、26+ subtests
- server/internal/api/handlers/device_handler.go: +47 行(3 個 firmware 衍生欄位)
- server/internal/api/router.go: +23 行
- server/main.go: +10 行(wire firmware service + handler)

4 endpoints 全到位(對齊 TDD §3.1):
- GET /api/devices: 加 firmwareIsLegacy / firmwareCanUpgrade / bundledFirmwareVersion(firmwareVersion 沿用既有 DeviceInfo 鍵)
- POST /api/devices/scan: 同步走 enrichDevices
- POST /api/devices/:id/firmware/upgrade: 202 + {taskId}
- GET /api/firmware/active-tasks: HasActiveTask + GetActiveTaskInfo
- WebSocket room firmware:<deviceID> broadcast 對齊 §4.2

關鍵設計:
- 3 層 interface(firmwareBroadcaster / firmwareService / deviceLookupSource)+ DeviceManagerAdapter 解 import cycle
- bundledVersion cache(只 cache success、避免 thundering herd / poison)
- isLegacyFirmware 對齊 bridge.py 規則(legacy_exact set + KDP1.x prefix + KDP2-9 forward-compat)+ parity 真值表測試
- 5 個錯誤碼齊全(DEVICE_NOT_FOUND / FW_UNSUPPORTED_CHIP / FW_DEVICE_BUSY / FW_UPGRADE_FAILED / FW_UPGRADE_BRICK_RISK)

Reviewer 兩輪審查:
- Round 1: 0 Critical / 1 Major / 3 Minor / 5 Suggestion
- Round 2: 0 Critical / 0 Major / 0 Minor / 3 極小 Suggestion(全部 backend 不需處理、純評估)
- Major 1(JSON 雙鍵衝突 firmwareVer vs firmwareVersion)方案 A 完全到位、3 個 test 鎖定 regression

TDD 同步:firmware-management.md §3.1 line 131 firmwareVer → firmwareVersion 對齊實作。

測試:go test ./... -race -count=1 全綠(handlers 2.489s / api 3.522s / ws 4.623s / device 1.931s / firmware 2.695s / driver/kneron 5.583s / model 5.022s)

SIGTERM main.go 整合留 M9-4.5(與 Wails OnBeforeClose 一起做)。

下一步:M9-4 Frontend Devices 頁 FW badge + 升級 modal + i18n(1.5 人天)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-25 12:05:42 +08:00

25 KiB
Raw Blame History

Reviewer Report — M9-3 API handler + WebSocket progress

  • 審查日期2026-05-25
  • 被審任務M9-3 (Backend) — API handlers + WebSocket broadcast + DeviceInfo 衍生欄位
  • TDD 對應v2/firmware-management.md §3 / §4.2 / §8.6.2
  • PRD 對應feature-firmware-management.md AC-FW-1 ~ AC-FW-1.7

TL;DR

整體實作扎實、5 軸大致都過。沒有 Critical1 個 MajorJSON schema 重複欄位)、3 個 Minor、5 個 Suggestion。Schema 對齊 TDD §3 / §4.2 一致WS goroutine lifecycle 與 cleanup race 設計清楚、test 涵蓋 26 subtests 含等待 broadcast 完成 + cleanup 完成、不依賴 sleep 固定時長。不阻擋 M9-4(前端 +1 Major請順手把 deviceInfo firmwareVersion vs derived firmwareVer 雙鍵的事傳達給 frontend 處理)。不需升 security agent建議 backend 第 2 輪處理 Major 1雙 JSON 鍵),其他 Minor / Suggestion 可進 backlog 不阻擋。

審查範圍

# 檔案 行數 性質
1 server/internal/api/handlers/firmware_handler.go 404新檔 4 endpoint + WS forward + 4 個 derived field helper
2 server/internal/api/handlers/firmware_handler_test.go 632新檔 26 subtests
3 server/internal/api/handlers/device_handler.go 197→238+41 enrichDevices wrap + 衍生欄位整合
4 server/internal/api/router.go 236→259+23 wire firmware handler + 2 新 route + nil-safe
5 server/main.go 381→391+10 wire firmware service + DeviceManagerAdapter + firmwareDir 解析

合計:~110 行非 test 變更 + 632 行 test 新增。

文件符合性檢查

PRD 功能符合性AC-FW-1 系列)

AC 條目 是否在 M9-3 範圍 實作位置 符合程度
AC-FW-1升級流程整體 AC 部分API 邊界) firmware_handler.go:UpgradeDevice 完全符合202 + taskID 對齊 TDD §3.1
AC-FW-1.4(失敗類型 + reason 細分) 部分API 邊界) service.go forwardhandler 不參與 reason 細分 透過 service forward、reason 正確透出
AC-FW-1.5StatusUpgrading 鎖其他操作) M9-2 service mutex 管 service.tracker.Create 防重複 重複 POST → 409 FW_DEVICE_BUSY
AC-FW-1.7timeout 護欄 KL520 60s / KL720 200s M9-2 service 包 timeout、M9-3 handler 不參與 service UpgradeTimeoutFor N/A不在 M9-3 範圍)
AC-FW-1.8(不觸發 watchServer Error 由 service 隔離 goroutine 保證 service goroutine + ws broadcast async handler 沒在 HTTP 連線上阻塞
AC-FW-1.9graceful shutdown 拒絕) TDD §3 註明留 M9-4 service.RequestShutdown / WaitForActiveTasks 已備、main.go wire 留 M9-4 N/ATDD line 56 明示)

TDD §3.1 4 endpoint 對齊評估

Endpoint TDD 規格 實作 結果
POST /api/devices/:id/firmware/upgrade 202 + {success, data:{taskId}} firmware_handler.go:UpgradeDevicehttp.StatusAccepted + {success:true, data:{taskId}}
GET /api/firmware/active-tasks TDD §8.6.2 schema{hasActive, tasks:[]} ListActiveTasks{success, data:{hasActive, tasks:[]}} tasks 已主動 nil→[] 避免 nulltest L403 覆蓋)
GET /api/devices 4 個衍生欄位 TDD §3.1 line 131firmwareVer / firmwareIsLegacy / firmwareCanUpgrade / bundledFirmwareVersion FirmwareDerivedFields 4 個 + embed DeviceInfo ⚠️ 見 Major 1(雙 JSON 鍵)
POST /api/devices/scan 同上 4 欄 同上 device_handler.go:ScanDevices 也走 enrichDevices 對稱、一致性 OK

GET /api/devices/:id/firmware/versionsPOST .../downgrade 屬 B2 階段 M9-11、不在 M9-3 範圍、handler 也未實作router 也未註冊)、正確。

TDD §4.2 progress event schema 對齊

firmware_progress WebSocket payload schema 對齊(透過 firmwareProgressMessage wrapper + embedded FirmwareProgress

欄位 TDD §4.2 名稱 實作 JSON 鍵 結果
type wrapper 加) type:"firmware_progress"
device_id device_id deviceIdtypes.go L35 ⚠️ TDD 寫 snake_case 但全專案統一 camelCase本 review 視為 TDD spec drafting issue 不算缺陷CLAUDE.md 沒明定 case style
stage stage stage
percent percent percent
direction direction direction
message message message
elapsed_ms / eta_ms elapsedMs/etaMs 同上
reason / raw_error / before_version / error_code 同上 reason/rawError/beforeVersion/errorCode
after_version / method 同上 afterVersion/method 在 done event 由 service forward

schema 完整覆蓋 TDD §4.2 列出的 14 個欄位。

🔴 Critical必須修復、阻擋交付

無。

🟠 Major必須修復、不阻擋初步測試

Major 1 — GET /api/devices response 出現「firmwareVersion + firmwareVer 雙重 JSON 鍵」、TDD §3.1 規定的鍵被 shadow 風險

位置device_handler.go:59-62deviceWithFirmware embed+ driver/interface.go:26FirmwareVer json:"firmwareVersion,omitempty"+ firmware_handler.go:234FirmwareDerivedFields.FirmwareVer json:"firmwareVer"

問題

  • DeviceInfo.FirmwareVer 既有 JSON tag 為 firmwareVersion(已既有 API contract
  • FirmwareDerivedFields.FirmwareVer 新 JSON tag 為 firmwareVerTDD §3.1 line 131 指定)
  • deviceWithFirmware 把兩個 struct embed 在一起 → Go JSON encoder 會同時輸出兩個 key,前端拿到的 device object 會有 firmwareVersion: "KDP" + firmwareVer: "KDP" 兩個欄位
  • TDD §3.1 line 131 明示 key 是 firmwareVer、但既有 frontend 多半在用 firmwareVersion
  • 沒寫測試覆蓋這個雙鍵情境firmware_handler_test.go 只測 DeriveFirmwareFields 回傳的 struct、沒測 enrichDevices 整體 JSON 輸出)

影響

  1. 前端拿到兩個語意相同但名稱不同的欄位、混淆且日後可能不一致(如:將來 device manager update FirmwareVer 但 derived 沒重算)
  2. Schema 文件無法清楚說明哪個是 source of truth
  3. TDD 規範的 firmwareVer 與既有 schema 的 firmwareVersion 並存沒法律性裁決
  4. JSON payload 變大device list 每個 entry +1 重複欄位)

建議修改:擇一:

方案 A最佳、最 minimal:在 deviceWithFirmware 用 inline 顯式定義替代 embedded、明確控制 JSON 欄位:

type deviceWithFirmware struct {
    driver.DeviceInfo                            // embed 既有欄位(含 firmwareVersion
    FirmwareIsLegacy       bool   `json:"firmwareIsLegacy"`
    FirmwareCanUpgrade     bool   `json:"firmwareCanUpgrade"`
    BundledFirmwareVersion string `json:"bundledFirmwareVersion"`
    // FirmwareVer 不再加(與 DeviceInfo.FirmwareVer 同義、避免重複)
}
  • 同時更新 TDD §3.1 line 131 把 firmwareVer 改回沿用 firmwareVersion 一致命名
  • enrichDevices 內把 entry.FirmwareDerivedFields = ... 改為 set 3 個欄位(不含 FirmwareVer

方案 B:保留 firmwareVer、改 DeviceInfo.FirmwareVer JSON tag 為 firmwareVer不建議、會破壞既有 frontend & 既有 device list API contract、影響範圍超出 M9 範圍)

方案 C(最小改動、最不好):在 deviceWithFirmware 加 MarshalJSON 自訂、刪除其中一個鍵。複雜且維護成本高。

強烈建議方案 A。同時請 backend 在 device_handler_test.go 加一個 TestEnrichDevices_NoFieldDuplication 測試JSON marshal 後檢查 firmwareVersion 出現次數 + firmwareVer 出現次數,避免日後 regression。

升級理由

  • 屬「API contract 對外可見」的 schema bug、影響 frontend M9-4 開發
  • M9-4 frontend 即將消費此 endpoint、若 frontend 已寫了 firmwareVer 認知、晚改成本更高
  • 雖然不會 crash、但屬 PRD/TDD 對齊問題(規格不一致由實作暴露)→ 須在合 PR 前裁決

🟡 Minor建議修復

Minor 1 — firmware_handler.go:140context.Background() 切斷 server shutdown propagation

位置firmware_handler.go:140

現況handler 用 context.Background() 傳給 svc.UpgradeFirmware(ctx, id, chip)理由註解清楚HTTP request ctx 回 202 後 cancel、不能 cover 整個升級流程。Service 內部用 UpgradeTimeoutFor(chip) + 30s 包 timeout、不會 leak goroutine。

問題:當 server 自己進 graceful shutdownM9-4 / SIGTERMBackground() ctx 不會跟著被 cancel。雖然 service 端有 RequestShutdown + WaitForActiveTasks 設計,但 ctx 級別沒法傳訊。TDD §8.6 設計是「等 task 完成」、不主動 cancel所以這個現況其實是符合設計意圖的、不是 bug。

建議:在 firmware_handler.go:140 註解補一句說明:「未來 graceful shutdown 不從 ctx 主動 cancel、靠 service.RequestShutdown + WaitForActiveTaskstask.cancel 預留給強制 force-cancel 路徑M9-4 範圍)」。讓未來讀 code 的人不會誤以為這是 bug。

Minor 2 — bundledVersionFor 在 firmwareDir 不存在時也會 cache "unknown",永遠 cache 不會 retry

位置firmware_handler.go:266-303

問題:第一次讀 firmware/<chip>/VERSION 失敗(檔案不存在 / 權限錯)→ cache "unknown" 永遠不會重試。如果這是 dev mode 下臨時還沒 build firmware、後續 build 完handler 不 restart 就永遠看不到 version。

影響dev 場景下小困擾、production 安裝包 firmware 一定 ship 進去、影響低。

建議:兩種選一:

  • (a) 對 "unknown" 不 cache、每次 list device 都 stat 一次cost 低、磁碟 stat ~µs 級)
  • (b) 加 TTL cache如 5 分鐘)、過期重 stat

實務上 (a) 較簡單、且 dev 場景頻率低。

Minor 3 — Test L302「TestUpgradeDevice_DeviceNotFound」斷 device not found 但 fake lookup 是回 "device not found: nope" 字串、實際 driver 端 device.Manager 的錯誤訊息格式可能不同

位置firmware_handler_test.go:111-116

現況fake lookup 自己造 error message "device not found: " + id。生產時用的 device.Manager.GetDevice 的錯誤訊息格式不一定相同test 沒 lock 真實格式)。

問題handler 沒解析 error 內容(直接 echo 到 response.error.message、所以功能上 OK。但 test 沒驗證 handler 對「真實 device.Manager 錯誤訊息」的處理一致性。

建議:補一個整合測試(或在 device package test 端)驗證 device.Manager.GetDevice("nope") 的 error 格式維持穩定。或在 handler 端 wrap error → 明確只回 "device not found: " + id、不暴露內部錯誤訊息(同時減少潛在 raw error 洩漏面、與 Security 軸關聯)。

💡 Suggestion非必要、改善建議

Suggestion 1 — firmwareIsLegacy 判定規則:與 bridge.py classify 邏輯一致性沒 cross-test

位置firmware_handler.go:340-350 isLegacyFirmware

現況handler 端的 isLegacyFirmware 規則文字描述清楚(含 KDP + 不含 KDP2 → legacy。但 bridge.pypython端的 firmware 字串分類邏輯沒在 Go side cross-test。

建議:加一個 testdata-driven test、列出 bridge.py 觀察到的 firmware 字串樣本(如 "KDP", "KDP1.0", "KDP2-v2.2.0", "v2.2.0", "2.2.0", ""+ 預期 isLegacy、由本 Go test 與 python classify 端對 input 保持一致。M9-5 三平台實機驗證時、若實機回報的 firmware 字串與 test 樣本不一致、要立即補進 test。

Suggestion 2 — forwardProgressToWS goroutinedone 後 sleep grace 給訂閱者 buffer

位置firmware_handler.go:164-177

現況service close(intermediate) → handler goroutine for ev := range progressCh 完 → 立即呼 CleanupTaskCleanupTask 移除 tracker entry → ListActiveTasks 立刻看不到此 task。

潛在問題WS room 訂閱者(前端)可能還沒收到最後 done event網路延遲、worker pool 還沒 dispatch就看到 GET /api/firmware/active-tasks 已經沒此 task → UI flickmodal 顯示「完成」前端就被告知「無 task」

建議:在 CleanupTask 前 sleep 500ms 給 broadcast buffer 飛完。或設計成 tracker entry 進「completed」狀態保留 30s、ListActiveTasks 預設只回 active、ListAll 才回 completed更乾淨、但複雜度提升。M9-4 frontend 整合時若觀察到此 race 再處理。

Suggestion 3 — classifyServiceError default fallback 是 FW_UPGRADE_FAILED 500、但「未知 error」可能是 ErrDeviceBusy / ErrInvalidArgument 之外的事

位置firmware_handler.go:354-369

現況default case 回 500 + FW_UPGRADE_FAILED。test L353 有驗 errors.New("boom") → 500 / FW_UPGRADE_FAILED。

建議:加 log即使 default、也要 log 完整 err、客服診斷用

default:
    log.Printf("warn: firmware service unknown error: %v", err)
    return http.StatusInternalServerError, "FW_UPGRADE_FAILED"

或更謹慎用 slog.Error

Suggestion 4 — device_handler.go:enrichDevices 在 fwHandler=nil 時、bundledFirmwareVersion 給 "unknown"、但 firmwareIsLegacy 給 zero value false

位置device_handler.go:74-78

現況fwHandler 為 nil 時、FirmwareDerivedFields{FirmwareVer: d.FirmwareVer, BundledFirmwareVersion: "unknown"}IsLegacy / CanUpgrade 走 zero value false

潛在問題legacy KDP1 device 在「firmware feature disabled」場景下、前端看到 isLegacy=false、CanUpgrade=false與 fwHandler 存在但 disabled 的情境語意不同(後者 isLegacy 可能是 true 但 CanUpgrade=false。前端無法區分「feature 不存在」與「device 不可升」。

建議:在 response 加一個 firmwareFeatureEnabled bool 欄位true 表示後端有 firmware service、false 表示沒有)、前端用它決定 UI 顯示「升級按鈕」or「無此功能」。M9-4 frontend 啟動時若需要區分再加;目前因為 production 一定有 firmwareSvc、不阻擋。

Suggestion 5 — Test 缺少「WS broadcast goroutine leak」直接驗證

位置firmware_handler_test.go 全檔

現況test 有 waitForBroadcasts + waitForCleanup 確認 broadcast / cleanup 都跑完。但沒直接驗證「forward goroutine 退出後沒留下 leak」。

建議:用 runtime.NumGoroutine() 在 test 開頭 + 結尾抓快照、若 diff > 1 fail。或用 go.uber.org/goleak如果專案有用。次要、M9-3 既有 test 已合理覆蓋一般 case。

4 endpoint 對齊 TDD §3.1 評估

Endpoint TDD 規格(行號) HTTP method URL Request body Response schema 實作 評估
Upgrade §3.1 L132 POST /api/devices/:id/firmware/upgrade {} 202 + {success, data:{taskId}} UpgradeDevice 全對齊
Active tasks §8.6.2 GET /api/firmware/active-tasks {success, data:{hasActive, tasks:[]}} ListActiveTasks 對齊、tasks nil→[] 處理
Devices list 衍生欄位 §3.1 L131 GET /api/devices data[].firmwareVer/firmwareIsLegacy/firmwareCanUpgrade/bundledFirmwareVersion enrichDevices wrap、但 firmwareVer 與既有 firmwareVersion 雙鍵共存Major 1 ⚠️ Major 1
Devices scan 衍生欄位 §3.1 L131同上 POST /api/devices/scan 同上 同上、走相同 enrichDevices ⚠️ Major 1

Endpoint count match4 endpoint 全部都在實作中。WS room firmware:<deviceID> 透過 forwardProgressToWS 正確 broadcast。

WS broadcast schema 對齊 §4.2

14 個欄位 + 1 個 type wrapper 鍵全部對齊。

firmwareProgressMessage 用 Go embedded struct firmware.FirmwareProgress 做 JSON 平坦展開、避免巢狀化、與 TDD §4.2 schema 對齊(前端在 message handler 直接讀 payload.deviceId / stage / percent / ... 不需 unwrap

verified by testL267-289 JSON round-trip 驗證 type:"firmware_progress", deviceId, stage, percent 都在L292-298 done event 驗證 afterVersion

26 subtests 品質評估

Test 區塊 Subtests 覆蓋 評估
TestUpgradeDevice_Success 1 完整 happy path + WS broadcast + cleanup
TestUpgradeDevice_DeviceNotFound 1 404 + 訊息含 DEVICE_NOT_FOUND
TestUpgradeDevice_UnsupportedChip 3 (KL630/KL730/unknown) handler 預先 filter、不打 service
TestUpgradeDevice_ServiceErrors 6 (busy/unsupported/brick/fail/unknown/not_found) 完整錯誤碼 mapping
TestListActiveTasks_Empty 1 hasActive=false + tasks=[](非 null
TestListActiveTasks_WithTasks 1 tasks 完整欄位 + EtaSeconds
TestDeriveFirmwareFields 8 KDP1/KDP1.0/KDP2/v2.2.0/空/KL720/KL630/unknown
TestDeriveFirmwareFields_EmptyFirmwareDir 1 fallback "unknown"
TestChipFromDeviceType 7 KL520/720/630/730 + case insensitive + unknown + empty

強項

  • 26 個 subtest、覆蓋面廣
  • waitForBroadcasts + waitForCleanup 而非 sleep 固定值、async-aware
  • mock 接口設計合理spyBroadcasterFW 抓所有 BroadcastToRoom 呼叫)
  • json round-trip 驗證 schema不只比較 struct

缺口

  • 沒測 enrichDevices 整體 JSON 輸出(雙鍵問題)→ Major 1 提的補測
  • 沒測 forward goroutine leak次要、Suggestion 5
  • 沒測 bundledVersionFor 對 "unknown" 的 cache 行為Minor 2
  • 沒測 multiple concurrent UpgradeDevice 同 device → service 應 reject 但 handler 沒測service test 端有測、handler 端可信任、不阻擋)

安全軸特別評估

風險 分析 結論
deviceID path traversal/injection c.Param("id") 直接傳入、沒 sanitize但用途只是 map key lookupdevice manager + WS room name 串接 → 不會打 file system / DB / SQL 不阻擋。但是建議 backend 加白名單檢查(如 device manager 端、deviceID 應符合特定 pattern— 此屬整體系統設計、不擋 M9-3
WS broadcast 洩漏內部資訊 rawError 包含 bridge.py 原始 traceback + 含 stack trace 風險 ⚠️ 已知設計、TDD §4.2 line 224 明文允許用於「客服診斷」;現況不算 bug、但建議 frontend 收到 rawError 時不要默認展示、只在使用者主動點「複製錯誤訊息」時才呈現(避免一般使用者看到 path / 內部訊息。Frontend M9-4 須注意
同 device 多人同時 POST upgrade service mutex 在 tracker.Create 內、嚴格 reject 第二個 不可繞過
chip 字串信任邊界 chip 從 device.Manager.GetDevice(id).Driver.Info().Type 取、不是 from request body 安全
confirmTokendowngrade 用) M9-3 不在範圍B2 階段 M9-11 N/A
secret/token 洩漏 無 secret 處理
用 path 跑 LimitReader(64) 讀 VERSION 檔 限制 64 bytes、避免讀大檔耗 良好實踐

結論:不需要升 security agent。rawError 設計是 TDD 既定(客服診斷必須)、由 frontend 負責不默認展示。

Concurrency 評估

WS broadcast goroutine leak risk

  • forwardProgressToWS goroutinefor ev := range progressCh 直到 progressCh close → service 在 runUpgrade 終態 defer close(task.ProgressCh) 保證 close → goroutine 必定退出
  • 終態後呼 CleanupTask 一次 → 不會 leak tracker entry

bundled version cache race

  • bundledMu RWMutex 保護 bundledVersions map → 多 goroutine 安全
  • 雙重檢查模式(先 RLock 讀 cache、miss 才 RLock release → 開 IO → 寫 cache有「雙 reader 同時 miss、都讀 disk」的 race
  • 但 outcome 是 cache 被相同值 overwrite、不會錯亂、屬可接受的 benign race
  • 無 data racego race detector 應該 clean

service mutex 是否被 endpoint 端繞過

  • handler 端沒有自己的 mutex、完全靠 service.tracker.Create 的 mutex
  • service.tracker.Create 在 mu 內 atomic CAS → 不可繞過
  • handler 同 device 兩個並發 POST → 都進到 service → service 第二個 reject 回 ErrDeviceBusy → handler classifyServiceError → 409
  • 保護完整

是否阻擋 M9-4

不阻擋。但 frontend 端需注意:

  1. Major 1雙 JSON 鍵):請 frontend 在實作前先確認用 firmwareVer 還是 firmwareVersion(建議用既有 firmwareVersion、待 backend 第 2 輪移除 derived firmwareVer 後對齊)。可先寫 frontend code 用 firmwareVersion、不會 break。
  2. WS event handler 注意:rawError 不要默認 render、只在「複製錯誤訊息」按鈕點擊時用安全軸建議
  3. firmwareFeatureEnabled 區分Suggestion 4目前不需處理、production 一定有 firmwareSvc。

是否升 security agent

無 auth 變更、無新對外 endpointloopback only、與既有 device API 同安全邊界)、無第三方整合、無 PII。rawError 屬已知設計、TDD 已聲明用途。Path param 沒打 file system / SQL。

是否需 backend 第 2 輪

。建議第 2 輪聚焦:

  1. 必修Major 1 — 解決 firmwareVersion / firmwareVer 雙鍵問題(建議方案 A刪 derived 的 FirmwareVer、TDD §3.1 line 131 同步改回 firmwareVersion)。同時補 device_handler 端的 enrichDevices JSON 輸出測試。
  2. 建議Minor 1 — handler L140 註解補充 ctx 設計意圖
  3. 可選Minor 2 / Suggestion 1-5 任選擇處理;不阻擋 M9-4 上工。

結論

維度 評分
Correctness Passschema 對齊、邊界 case 完整、async 行為正確)
Readability Pass命名一致、註解豐富、結構與既有 handler 對齊)
Architecture Passinterface 隔離、DeviceManagerAdapter 解循環依賴、nil-safe wire
Security Pass粗篩、無需深審
Performance Passcache + LimitReader + goroutine lifecycle 都合理)
Test Pass26 subtests 覆蓋面廣、async-aware
文件對齊 ⚠️ Major 1JSON schema 雙鍵)

整體:⚠️ 需修改後通過1 個 Major 必修、不阻擋 M9-4 frontend 平行起步、但合 PR 前 backend 第 2 輪解決 Major 1

優點(明確讚美)

  • firmwareBroadcaster + firmwareService + deviceLookupSource 三層 interface 隔離 + DeviceManagerAdapter 解循環依賴的設計非常乾淨、test 可輕鬆 mock、不拉 device package 進 firmware package、不拉 ws package 進 firmware package、不拉 firmware package 進 device package。M9-2 與 M9-3 之間的 contract 透過介面清楚定義、未來換實作(如真實 device 換 mock device、ws.Hub 換 NATS成本極低。
  • Test 26 subtests 用 waitForBroadcasts + waitForCleanup 取代 sleep 固定時長、明顯是有經驗的 async-aware 寫法、不會因 CI 慢而 flaky。
  • tasks nil→[] 處理(避免 frontend 拿到 null 還要判斷)+ JSON round-trip 驗證 schema、體貼 frontend、預先想好 schema breakage 防護。

Verification 自評

A 層(每個 review 必做)

  • R-A15 軸 + 測試軸全跑過、每軸實質判斷 ≥ 20 字
  • R-A2文件符合性 checklist 完整PRD AC-FW-1 系列、TDD §3.1 4 endpoint、TDD §4.2 schema 三張表都填)
  • R-A3Major 1 附 line number + 規則名稱JSON schema 雙鍵)+ 三個具體建議方案
  • R-A4寫明優點 ≥ 1 條(實際 3 條)
  • R-A5本次無不確定項明示
  • R-A6§12.2 通用 6 條 evidence — 無 silent failuresservice 端 error event 補 push、不吞、無 dead codetask.cancel 預留但有註解說明 M9-4 用途)、無 hardcoded secrets、無 unsafe HTML/SQL、doc 同步狀態TDD §3.1 line 131 與實作對 Major 1 提請更新)、被審 commit 是否 clean未檢查、屬被審 agent 責任、不在本 review evidence 範圍)

B 層milestone / 大 PR

  • 本次審查單一 milestone (M9-3) 含 5 個檔案、~110 行非 test 變更、不到大 PR 閾值500 行)。暫緩 B 層 verification
  • 本次 reviewM9-3 / 5 個檔案 / 中等規模
  • 暫緩原因:單一 milestone、非跨領域、非大 PR
  • 預計補做時機M9-5 testing 階段A 階段三平台驗收)會做整 A 階段跨檔比對
  • 風險評估:低(接下來 M9-4 frontend 是接口消費端、會自然驗證 backend schema 一致性)

C 層PR / 合併前最終 review

  • 不適用M9-3 不是 PR / merge 階段、是 milestone review