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>
25 KiB
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 軸大致都過。沒有 Critical、1 個 Major(JSON 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 forward;handler 不參與 reason 細分 |
✅ 透過 service forward、reason 正確透出 |
| AC-FW-1.5(StatusUpgrading 鎖其他操作) | ❌ M9-2 service mutex 管 | service.tracker.Create 防重複 | ✅ 重複 POST → 409 FW_DEVICE_BUSY |
| AC-FW-1.7(timeout 護欄 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.9(graceful shutdown 拒絕) | ❌ TDD §3 註明留 M9-4 | service.RequestShutdown / WaitForActiveTasks 已備、main.go wire 留 M9-4 | N/A(TDD line 56 明示) |
TDD §3.1 4 endpoint 對齊評估
| Endpoint | TDD 規格 | 實作 | 結果 |
|---|---|---|---|
POST /api/devices/:id/firmware/upgrade |
202 + {success, data:{taskId}} |
firmware_handler.go:UpgradeDevice 回 http.StatusAccepted + {success:true, data:{taskId}} |
✅ |
GET /api/firmware/active-tasks |
TDD §8.6.2 schema:{hasActive, tasks:[]} |
ListActiveTasks 回 {success, data:{hasActive, tasks:[]}} |
✅ tasks 已主動 nil→[] 避免 null(test L403 覆蓋) |
GET /api/devices 4 個衍生欄位 |
TDD §3.1 line 131:firmwareVer / 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/versions 與 POST .../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 |
deviceId(types.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-62(deviceWithFirmware embed)+ driver/interface.go:26(FirmwareVer json:"firmwareVersion,omitempty")+ firmware_handler.go:234(FirmwareDerivedFields.FirmwareVer json:"firmwareVer")
問題:
DeviceInfo.FirmwareVer既有 JSON tag 為firmwareVersion(已既有 API contract)FirmwareDerivedFields.FirmwareVer新 JSON tag 為firmwareVer(TDD §3.1 line 131 指定)deviceWithFirmware把兩個 struct embed 在一起 → Go JSON encoder 會同時輸出兩個 key,前端拿到的deviceobject 會有firmwareVersion: "KDP"+firmwareVer: "KDP"兩個欄位- TDD §3.1 line 131 明示 key 是
firmwareVer、但既有 frontend 多半在用firmwareVersion - 沒寫測試覆蓋這個雙鍵情境(firmware_handler_test.go 只測
DeriveFirmwareFields回傳的 struct、沒測enrichDevices整體 JSON 輸出)
影響:
- 前端拿到兩個語意相同但名稱不同的欄位、混淆且日後可能不一致(如:將來 device manager update
FirmwareVer但 derived 沒重算) - Schema 文件無法清楚說明哪個是 source of truth
- TDD 規範的
firmwareVer與既有 schema 的firmwareVersion並存沒法律性裁決 - 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:140 用 context.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 shutdown(M9-4 / SIGTERM)、Background() ctx 不會跟著被 cancel。雖然 service 端有 RequestShutdown + WaitForActiveTasks 設計,但 ctx 級別沒法傳訊。TDD §8.6 設計是「等 task 完成」、不主動 cancel;所以這個現況其實是符合設計意圖的、不是 bug。
建議:在 firmware_handler.go:140 註解補一句說明:「未來 graceful shutdown 不從 ctx 主動 cancel、靠 service.RequestShutdown + WaitForActiveTasks,task.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.py(python)端的 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 goroutine:done 後 sleep grace 給訂閱者 buffer
位置:firmware_handler.go:164-177
現況:service close(intermediate) → handler goroutine for ev := range progressCh 完 → 立即呼 CleanupTask。CleanupTask 移除 tracker entry → ListActiveTasks 立刻看不到此 task。
潛在問題:WS room 訂閱者(前端)可能還沒收到最後 done event(網路延遲、worker pool 還沒 dispatch),就看到 GET /api/firmware/active-tasks 已經沒此 task → UI flick(modal 顯示「完成」前端就被告知「無 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 match:4 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 test:L267-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 lookup(device 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 |
✅ 安全 |
| confirmToken(downgrade 用) | 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
forwardProgressToWSgoroutine:for ev := range progressCh直到 progressCh close → service 在runUpgrade終態defer close(task.ProgressCh)保證 close → ✅ goroutine 必定退出- 終態後呼
CleanupTask一次 → ✅ 不會 leak tracker entry
bundled version cache race
bundledMuRWMutex 保護bundledVersionsmap → ✅ 多 goroutine 安全- 雙重檢查模式(先 RLock 讀 cache、miss 才 RLock release → 開 IO → 寫 cache)有「雙 reader 同時 miss、都讀 disk」的 race
- 但 outcome 是 cache 被相同值 overwrite、不會錯亂、屬可接受的 benign race
- ✅ 無 data race(go 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 端需注意:
- Major 1(雙 JSON 鍵):請 frontend 在實作前先確認用
firmwareVer還是firmwareVersion(建議用既有firmwareVersion、待 backend 第 2 輪移除 derivedfirmwareVer後對齊)。可先寫 frontend code 用firmwareVersion、不會 break。 - WS event handler 注意:
rawError不要默認 render、只在「複製錯誤訊息」按鈕點擊時用(安全軸建議)。 firmwareFeatureEnabled區分(Suggestion 4)目前不需處理、production 一定有 firmwareSvc。
是否升 security agent
否。
無 auth 變更、無新對外 endpoint(loopback only、與既有 device API 同安全邊界)、無第三方整合、無 PII。rawError 屬已知設計、TDD 已聲明用途。Path param 沒打 file system / SQL。
是否需 backend 第 2 輪
是。建議第 2 輪聚焦:
- 必修:Major 1 — 解決
firmwareVersion/firmwareVer雙鍵問題(建議方案 A:刪 derived 的 FirmwareVer、TDD §3.1 line 131 同步改回firmwareVersion)。同時補device_handler端的 enrichDevices JSON 輸出測試。 - 建議:Minor 1 — handler L140 註解補充 ctx 設計意圖
- 可選:Minor 2 / Suggestion 1-5 任選擇處理;不阻擋 M9-4 上工。
結論
| 維度 | 評分 |
|---|---|
| Correctness | ✅ Pass(schema 對齊、邊界 case 完整、async 行為正確) |
| Readability | ✅ Pass(命名一致、註解豐富、結構與既有 handler 對齊) |
| Architecture | ✅ Pass(interface 隔離、DeviceManagerAdapter 解循環依賴、nil-safe wire) |
| Security | ✅ Pass(粗篩、無需深審) |
| Performance | ✅ Pass(cache + LimitReader + goroutine lifecycle 都合理) |
| Test | ✅ Pass(26 subtests 覆蓋面廣、async-aware) |
| 文件對齊 | ⚠️ Major 1(JSON 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。 tasksnil→[] 處理(避免 frontend 拿到 null 還要判斷)+ JSON round-trip 驗證 schema、體貼 frontend、預先想好 schema breakage 防護。
Verification 自評
A 層(每個 review 必做)
- R-A1:5 軸 + 測試軸全跑過、每軸實質判斷 ≥ 20 字
- R-A2:文件符合性 checklist 完整(PRD AC-FW-1 系列、TDD §3.1 4 endpoint、TDD §4.2 schema 三張表都填)
- R-A3:Major 1 附 line number + 規則名稱(JSON schema 雙鍵)+ 三個具體建議方案
- R-A4:寫明優點 ≥ 1 條(實際 3 條)
- R-A5:本次無不確定項(明示)
- R-A6:§12.2 通用 6 條 evidence — 無 silent failures(service 端 error event 補 push、不吞)、無 dead code(task.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。
- 本次 review:M9-3 / 5 個檔案 / 中等規模
- 暫緩原因:單一 milestone、非跨領域、非大 PR
- 預計補做時機:M9-5 testing 階段(A 階段三平台驗收)會做整 A 階段跨檔比對
- 風險評估:低(接下來 M9-4 frontend 是接口消費端、會自然驗證 backend schema 一致性)
C 層(PR / 合併前最終 review)
- 不適用(M9-3 不是 PR / merge 階段、是 milestone review)。