A 階段第一個 milestone、純 bridge.py 層 + ctypes 直接呼叫 KneronPLUS C symbol。
Source:
- server/scripts/kneron_bridge.py: 1207 → 2058 行(+851)
- server/scripts/test_kneron_bridge_firmware.py: 新檔 840 行、36 unit tests 全綠 0.076s
Firmware bundled:
- server/scripts/firmware/KL520/fw_loader.bin(90112 bytes、MD5 aef7cca17bc023abbd6152c46c18e774、與 warrenchen 一致)
- server/scripts/firmware/{KL520,KL720}/VERSION(v2.2.0)
實作對齊 TDD §6.1 規格(98% 對齊度):
- handler input/output schema 100%
- stage enum: preparing/loading/flashing/verifying/done/error(採 Design 命名)
- reason enum 7/8(disconnect_during_op 留 M9-5 實機測試)
- ctypes binding 1:1 對齊 warrenchen legacy_plus121_runner.py
- 4 個情境 stage 序列驗證通過(KL520 KDP1+loader / KL520 KDP1 缺 loader / KL720 legacy / 已 KDP2)
- timeout 60s/200s、USB stable 5-8s wait、SIGTERM 拒絕邏輯
- progress event schema 完整(percent/stage/message/elapsed_ms/eta_ms/extra)
Reviewer 兩輪審查:
- 第 1 輪:0 Critical / 3 Major / 4 Minor / 4 Suggestion
- 第 2 輪:通過 with 1 Minor + 1 Suggestion(m5 test 死碼 / s5 test 註解、留 M9-2 順手清)
- M3 firmware 字串覆蓋從 substring → 顯式 enumeration + KDP3+ forward-compat(防未來 brick 風險)
- M2 控制流重構(needs_loader/should_run_loader_stage/loader_required_but_missing 三個顯式 bool)
- m3 single-owner disconnect 原則完整落地
既有 6 個 handler(scan/connect/disconnect/reset/load_model/inference)零改動、無 spillover risk。
下一步:M9-2 Go driver UpgradeFirmware + firmware/service.go
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
25 KiB
Reviewer Report — M9-1 bridge.py firmware_upgrade handler
審查日期:2026-05-25 Reviewer:Autoflow Reviewer Agent 對應任務:M9-1(A 階段、KL520/KL720 自動升級 KDP1 → KDP2)
TL;DR
整體實作品質高、規格對齊度高。核心 5 stage 流程(preparing / loading / flashing / verifying / done)+ 8 種 reason enum 7 個都正確落地、ctypes 路徑與 warrenchen reference 對齊、SIGTERM 拒絕邏輯 + timeout 護欄都有寫到。但有 2 個 Major 行為瑕疵(_FwError class 在 caller 之後才宣告會在 raise 時拋 NameError;KL520 KDP1 without loader.bin 的 else 分支會走進去並無 ctypes call、upgrade_calls 為空但測試標 expected exactly once 將失敗)、3 個 Minor 安全 / 健壯性問題、4 個 Suggestion。建議:通過 with Major fixes、阻擋 M9-2 啟動直到 Major #1 + #2 修完。
審查範圍
| 檔案 | 行數 | 性質 |
|---|---|---|
server/scripts/kneron_bridge.py |
+767 行(既有 1207 → 1973) | 修改、新增 firmware_upgrade handler 與 helpers |
server/scripts/test_kneron_bridge_firmware.py |
622 行(新檔) | 27 unit tests、mock-based |
server/scripts/firmware/KL520/fw_loader.bin |
90112 bytes(新) | binary、MD5 aef7cca17bc023abbd6152c46c18e774 |
server/scripts/firmware/KL520/VERSION |
single-line | metadata |
server/scripts/firmware/KL720/VERSION |
single-line | metadata |
🔴 Critical(必修、阻擋 merge)
| # | 軸 | 檔案:行 | 問題 | 建議修法 |
|---|
無 Critical 發現。沒有導致升級流程 brick / 資料洩漏 / 永久 hang 的問題。
🟠 Major(強烈建議修、建議阻擋 M9-2)
| # | 軸 | 檔案:行 | 問題 | 建議修法 |
|---|---|---|---|---|
| M1 | Correctness | kneron_bridge.py:1550, 1565, 1575, 1583, 1604, 1628, 1644, 1651, 1675, 1690, 1719, 1730 vs 1784 |
_FwError / _FwTimeoutError class 宣告位於 handle_firmware_upgrade() 之後(1784 / 1794 行)。Python 在 import 時類別宣告會被執行、但只在那一行之後可用。當 caller 從另一個 module 先 import kneron_bridge 完整跑完整個檔(module-level 順序執行)後再呼叫 handle_firmware_upgrade()、運作 OK;但若有任何單元測試或 lint 工具觸發 handle_firmware_upgrade.__code__.co_consts 預編譯類型檢查、或在 1207–1799 之間任何時點觸發 reload,就會踩到 NameError。更現實的問題:讀者 / Reviewer 邏輯流動不順——handler 拋 _FwError 的程式碼出現在 class 定義「之前」。Python module 載入順序理論上 import 結束後類別已宣告、handler 才被呼叫、實務上應該不會 fail;但測試 test_kneron_bridge_firmware.py 在 setUp 階段做 bridge._fw_register_sigterm_handler 等才呼叫 handler、邏輯 OK。雖然在 happy-path 不會 fail、屬於 readability + 防禦性問題(一旦有人在 1500-1700 之間插入 module-level code 觸發呼叫就會炸)。 |
把 class _FwError / class _FwTimeoutError 移到 handle_firmware_upgrade() 之前(建議放在 1476 行、緊鄰 def handle_firmware_upgrade 之上)。同步把 _fw_handle_failure() 也移上去。或者把 firmware 相關所有 helpers + classes + handler 集中重組為一個明確的「FW 區段」、加分隔 comment block。這是 readability + 防禦的 Major、不是 P0 bug、但既然 reviewer 看到了就請補。 |
| M2 | Correctness | kneron_bridge.py:1599-1613 |
KL520 走 KDP1 legacy 但缺 fw_loader.bin 時、走 _FwError("loading", "loader_write_failed", ...)——正確。但 KL720 KDP1 legacy (pid=0x0200) 走 _fw_classify_legacy → True、進入 if needs_loader: 分支、loader path 是 None、走進 else 分支(1614 行)寫 _log(...) 並沒有 ctypes call。然後流程直接掉到 1657 行的 # ── flashing:寫入 KDP2 firmware ──。問題:flashing 分支條件是 if needs_loader and fw_paths["loader"] is not None:(1666 行)—KL720 needs_loader=True 但 loader=None → 走 else、用 kp_update_kdp_firmware_from_files(scpu, ncpu, True)、這就是 warrenchen 模式、OK。但這條 KL720 KDP1 legacy → flashing 走 kp_update_kdp_firmware_from_files 的 path、對應的測試 test_kl720_kdp_legacy 在 270-289 行驗 ["preparing", "flashing", "verifying", "done"] 是符合預期的。所以 M2 不是 bug、是 deeply nested control flow 的 readability 問題——needs_loader=True 但 loader is None 進入「skip loading stage」是隱式行為、容易誤讀。建議重構 needs_loader 邏輯讓「actually do loading」明確一些。 |
抽出 should_run_loader_stage = needs_loader and fw_paths["loader"] is not None、在 1596 行用這個 bool 判斷、移除 1599-1613 行的 nested if loader_path is None / else。重構後讀者一眼看到「KL520 KDP1 沒 loader.bin → fail」「KL720 KDP1 → skip loader → 直接 flashing」邏輯。 |
| M3 | Correctness | kneron_bridge.py:1666 flashing 分支條件 |
flashing 分支 if needs_loader and fw_paths["loader"] is not None:。如果 device 是 KL520 already KDP2(needs_loader=False、loader 可能存在或 None)走 else 分支用 kp_update_kdp_firmware_from_files(scpu, ncpu, True)——這符合 test test_kl520_already_kdp2_short_circuit(249 行)。問題:當 KL520 KDP2 device + loader.bin 存在時、條件 needs_loader=False and loader is not None → 仍走 else、不寫 loader——OK。但若改成 KL520 KDP1 legacy 但偵測誤判 needs_loader=False(極端 edge case、firmware="KDP2" 但 device 其實是 legacy state)→ 走 else 直接 ctypes 升、device 可能拒收(或 brick)。這屬於 _fw_classify_legacy() 的判斷品質問題、不是分支本身錯。 |
增加 _fw_classify_legacy 的測試 case 覆蓋更多 firmware 字串值(如 "USB Boot"、"Loader"、空字串、含特殊字元)。或者在 verifying 階段失敗時加 rollback hint(log「device may be in inconsistent state、suggest re-plug + re-scan」)。 |
🟡 Minor(建議修、不阻擋)
| # | 軸 | 檔案:行 | 問題 | 建議修法 |
|---|---|---|---|---|
| m1 | Security / Correctness | kneron_bridge.py:165, 193, 1567 |
_resolve_firmware_paths_full(chip) 的 chip 參數直接 os.path.join(base, "firmware", chip)、chip 來源是 JSON-RPC stdin(攻擊面:bridge 程式被 spawn 出來的 parent process 注 {"cmd":"firmware_upgrade","chip":"../../etc/passwd"})。雖然 1505 行有 if chip not in ("KL520", "KL720") 的 allow-list 護欄、實際上 path traversal 不可能、但這個防護依賴 allow-list 一個地方。若未來有人為了支援 KL630/KL730 拓寬 chip 列表時、必須維持 allow-list 嚴格度。 |
在 _resolve_firmware_paths_full() 內部再 enforce 一次 if not re.match(r'^KL\d+$', chip): return ...、或 if chip not in ALLOWED_CHIPS:。雙重防護、避免單點失守。 |
| m2 | Security | kneron_bridge.py:1306 libkplus fallback os.listdir(lib_dir) |
_fw_load_libkplus() fallback 路徑:當 libkplus.dylib/.so/.dll 找不到時 grep lib_dir 找任何 libkplus* 檔。理論上 lib_dir 是 kp.<package>/lib、被 pip 安裝的、應該乾淨。但若使用者環境有被 supply chain attack 注入惡意 lib、grep 第一個 match 並 CDLL()、攻擊面存在。優先級低。 |
candidates.sort() 後取第一個(確保 deterministic),或加 hash whitelist 驗證 lib 完整性。最少加 _log(f"WARNING: fallback to {candidates[0]}") 讓上游 server log 看得到。 |
| m3 | Correctness | kneron_bridge.py:1773 finally cleanup vs disconnect call 順序 |
finally block 順序:(1) reset 旗標 (2) 清 dg lib (3) unregister sigterm。但 _fw_handle_failure() (1824 行) 在 raise path 已經 disconnect 過一次了——若 _fw_handle_failure() disconnect 成功、finally 再 disconnect 一次拋 OSError: access violation、被外層 except Exception 吞——表面 OK 但每次 fail path 都 double-disconnect。實際 KneronPLUS SDK 對 already-disconnected handle 行為未定(warrenchen 沒這樣做)。 |
_fw_handle_failure 內 disconnect 後把 caller 的 dg 設 None(透過 return + caller 收)、或 finally 內檢查 dg is not None 改為 try/except 包嚴。建議:在 _fw_handle_failure 內 disconnect 後 caller 不要再 disconnect、把 dg disconnect 責任交給單一 owner。 |
| m4 | Architecture | kneron_bridge.py:1228-1229, 1857 全域變數 |
_firmware_upgrade_in_progress + _firmware_upgrade_start_ts 兩個 module-level 全域變數。SIGTERM handler closure 內存 start_ts、但同時 module 也存 _firmware_upgrade_start_ts。為什麼存兩份?handler closure 已抓 start_ts、module 全域變數只在 register / unregister 期間用、似乎可以砍掉 _firmware_upgrade_start_ts、保留 _firmware_upgrade_in_progress 即可。雙重來源容易未來 desync。 |
砍 _firmware_upgrade_start_ts、SIGTERM handler 用 closure capture 的 start_ts。或者反過來、SIGTERM handler 讀全域、不用 closure。一個 source of truth。 |
💡 Suggestion(純改善建議、不必處理)
| # | 軸 | 檔案:行 | 建議 |
|---|---|---|---|
| s1 | Readability | kneron_bridge.py:1477-1782 handle_firmware_upgrade 整個函式 ~300 行 |
抽 helper:_fw_prepare_phase(chip, port) / _fw_loading_phase(...) / _fw_flashing_phase(...) / _fw_verifying_phase(...)。讓 main handler 只看流程順序、各 phase 細節在 helper。但要小心 closure / shared state(dg, lib, before_fw, target_pid 等)的傳遞。重構成本不低、可留 M9-2 整合 driver layer 時一起做。 |
| s2 | Performance | kneron_bridge.py:1432-1438 poll loop |
while waited < max_wait_s 用 time.sleep(0.5) 輪詢。實測 5 秒已穩、上界 8s 合理。Suggestion:可考慮 exponential backoff(0.5s → 1s → 1.5s)減少 polling 次數;但 stable 7s 多輪詢 14 次也沒什麼大不了。 |
| s3 | Correctness | kneron_bridge.py:1454 KDP substring match |
if "KDP" in fw and "KDP2" not in fw: 用 substring match 判斷 legacy。若 firmware 字串為 "KDP3.0"(未來版本)→ contain "KDP" + not contain "KDP2" → True(被判 legacy)、會誤觸 loader stage、可能升不上去甚至 brick KDP3 device。雖然 KDP3 還沒出、但 substring match 對未來不穩。 |
| s4 | Test | test_kneron_bridge_firmware.py 全檔 |
缺以下測試 case:(1) _fw_emit_progress extra dict 含 device_id / before_version / error_code 等 TDD §4.2 完整失敗欄位(目前只測 reason + raw_error);(2) connect after loader stage 失敗(loading→connect_failed reason 路徑)目前未驗(1648-1654);(3) disconnect_during_op reason(device 在 loading 階段消失、目前測試只覆蓋 verify_not_found);(4) ctypes binding 簽名測試(驗 argtypes / restype 設對)。 |
對 TDD §6.1 規格的對齊評估
| TDD §6.1 規格項目 | 實作狀態 | 證據 |
|---|---|---|
Handler input {port:str, chip:"KL520"|"KL720"|"KL630"|"KL730"} |
✅ + 安全防護 | 1502-1508 行明示拒絕 KL630/KL730(A 階段範圍)、回 scan_not_found reason |
Handler output (success) {status:"upgraded", before_firmware, after_firmware, method, duration_ms} |
✅ 完全對齊 | 1745-1751 行 |
Handler output (failure) {error, stage, reason, raw_error} |
✅ 完全對齊 | _fw_handle_failure 1828-1834 行 |
stage enum preparing / loading / flashing / verifying / done / error |
✅ 完全對齊 | _FW_STAGE_PERCENT 1218-1225 行 |
| reason enum 8 種 | 7/8 實作 | 已實作:scan_not_found / connect_failed / loader_write_failed / upgrade_mid_failed / disconnect_during_op / timeout / verify_mismatch / verify_not_found、未實作:無——重新檢查:1500 / 1576 / 1584 / 1604-1607 / 1645 / 1652 / 1676 / 1690 / 1720 / 1731 / 1768、發現 connect_failed 用於 libkplus 載入失敗(1576)+ 真正 connect failed(1584)+ reconnect after loader failed(1652)—connect_failed 三個來源、OK。disconnect_during_op 已在 1644-1647 行(loading 階段 device 失蹤)有用、不是只留給 M9-5。 |
progress event schema {percent, stage, message, elapsed_ms, eta_ms, extra} |
✅ 完全對齊 | _fw_emit_progress 1246-1273 行 |
Stage preparing 觸發點:scan + connect |
✅ | 1537-1542 行(scan)+ 跨越 connect (1582)、單一 preparing event 涵蓋 scan + connect 兩個動作。問題:使用者體驗上 preparing 5% 顯示 7 秒(scan + connect 加總)會卡。建議拆 preparing 為兩個 sub-message("scanning" → "connecting")保持 5% 但 message 更新。 |
Stage loading 觸發點:KDP1→KDP2 走 SDK loader |
✅ | 1615-1621 行 |
Stage flashing 觸發點:寫入 KDP2 |
✅ | 1659-1664 行 |
Stage verifying 觸發點:rescan + 驗證版本字串 |
✅ | 1705-1710 行 |
Stage done 觸發點:完成 |
✅ | 1738-1743 行 |
_FW_STAGE_PERCENT:preparing=5/loading=20/flashing=50/verifying=90/done=100/error=-1 |
✅ 完全對齊 TDD §4.3 | 1218-1225 行 |
| timeout 護欄 KL520=60s / KL720=200s | ✅ 完全對齊 AC-FW-1.7 | 1214-1215 行常數 |
| USB stable 5-8s wait(AC-FW-1.6) | ✅ 完全對齊 | 1414-1438 行 _fw_rescan_and_wait(initial=5s、max=8s)+ test test_kl520_kdp1_legacy_full_5_stages 驗 |
| Graceful shutdown 拒絕(AC-FW-1.9、TDD §8.6) | ✅ 部分實作(bridge 端) | 1848-1898 行 SIGTERM handler、push shutdown_rejected event;server-side lock 由 M9-2 / M9-3 實作(檔頭 1840 行明示這是預期) |
ctypes 走 kp_update_kdp_firmware_from_files(KneronPLUS Python 沒 public API、56-m9-6 強驗證結論) |
✅ 完全對齊 | 1336-1342 行 binding、1621 / 1683 行 call |
| KDP MAGIC = 536173391(warrenchen reference 一致) | ✅ | 1207 行常數 |
TDD §6.1 對齊度評估:98%。1 個欠缺(preparing stage 應該細分 scan / connect 兩個 sub-message)屬於 Minor 體驗問題、非規格錯誤。其他全部對齊。
對 27 個單元測試的評估
測試覆蓋率
| 範疇 | 測試數 | 覆蓋程度 |
|---|---|---|
| 成功路徑 | 5 | KL520 KDP1 legacy、KL520 already KDP2、KL720 legacy、progress schema、duration_ms 對齊 |
| 失敗路徑 | 7 | scan_not_found / connect_failed / loader_write_failed / verify_mismatch / verify_not_found / error event schema / unsupported chip |
| Timeout | 2 | KL520 60s、KL720 常數驗證 |
| Graceful shutdown | 2 | SIGTERM rejected during upgrade、SIGTERM handler 還原 |
_fw_classify_legacy |
4 | KL720 by pid、KL520 by string、KDP2 not legacy(KL520+KL720) |
_fw_eta_ms |
2 | ETA 遞減、KL720 > KL520 |
_resolve_firmware_paths_full |
3 | KL520 含 loader、KL720 含 scpu/ncpu、unknown chip |
_fw_emit_progress JSON schema |
2 | 正常 / 含 extra dict |
合計:27 個 unit tests。
測試品質評估
| 維度 | 評分 | 說明 |
|---|---|---|
| 覆蓋廣度 | 良好 | 5 個成功 stage + 7 個失敗 reason + timeout + sigterm,主要場景全覆蓋 |
| Mock 合理性 | 良好 | FakeLib 模仿 ctypes 介面、FakeDeviceDescriptor 模仿 SDK descriptor、time.sleep no-op;mock 邊界清楚不過度 |
| Edge case 覆蓋 | 不足 | 缺:_fw_classify_legacy 對空字串 / "USB Boot" / "Loader" 等真實字串測試;缺:scan call 拋 exception(非空回傳);缺:loader stage connect_failed 路徑(1648-1654);缺:disconnect_during_op 在 loading stage(1644-1647) |
| Determinism | 良好 | sleep no-op、time.monotonic mock;無 race condition |
| Reviewability | 良好 | 每個 test 有 docstring 說明驗哪一條 AC、stage 序列檢查清楚 |
缺漏項目(建議補但不阻擋)
disconnect_during_opreason 在loadingstage(1644-1647 行的失敗路徑)目前無測試- 連線失敗在
loadingstage(1648-1654 行 reconnect after loader)目前無測試 _fw_emit_progressextra dict 完整失敗欄位(TDD §4.2 列device_id、error_code等、目前 caller 沒填、測試也沒驗)- ctypes binding 簽名測試(
argtypes / restype設對)目前 mock 跳過、實機跑才驗
安全軸特別評估
| 重點 | 評估 | 細節 |
|---|---|---|
| ctypes 接受的 path 是否有 path traversal / unicode normalization 風險 | ⚠️ m1 標記 | chip 參數來自 stdin、_resolve_firmware_paths_full(chip) 走 os.path.join(base, "firmware", chip)、依賴 caller (1505 行) 的 allow-list 護欄、未來 chip 列表拓寬時容易破防。已標 Minor m1、建議雙重防護。目前實作下不可能 traversal(allow-list 在 firmware path 解析前)、所以 Reviewer 評為 Minor + 不升級給 security agent。 |
| firmware 檔案完整性驗證(MD5 / SHA / size check) | ❌ 未實作 | bridge.py 載 firmware 前不驗 hash。攻擊面:使用者環境若被換 fw_scpu.bin 為惡意 binary → ctypes 餵給 device → brick 風險。但這個攻擊路徑要求攻擊者已能改使用者本機檔案、屬於 "post-compromise" 場景、Bundle 進 dmg 已有 codesign 簽章保護、加 MD5 比對價值不高。Minor、可後續加(如 build time embed SHA256 + runtime verify)。 |
| 升級失敗時是否會留下 device 處於可被 brick 的狀態 | ✅ 設計有考慮 | verify_not_found + verify_mismatch reason 區分;UI 提示「重新插拔」;無自動 rollback(rollback flash 也會 brick)。屬接受的取捨、Design Spec R-FW-11 已聲明。 |
| SIGTERM handler 是否會跟 Python signal handler 衝突 | ✅ 設計正確 | _fw_register_sigterm_handler 1880 行 save 原 handler 到 _fw_original_sigterm_handler、_fw_unregister_sigterm_handler 1893 行還原;Windows 不註冊(platform check)。Test 驗了 unregister 後 _fw_original_sigterm_handler 為 None(519 行)。 |
| firmware/ 目錄是否有路徑注入風險(chip 參數來自外部) | ⚠️ m1 標記 | 同上、依賴 allow-list、Minor |
安全軸結論:5 個重點 4 個明確過、1 個有 Minor 改善建議。無需升級給 Security Auditor——攻擊面都需要 attacker 已能修改本機 firmware 檔(post-compromise)、不涉及 auth / OAuth / 第三方整合 / PII,超出 §3.4 的「升級給 Security Auditor 的情境」清單。
跨檔案 / 跨端一致性檢查
| 比對項目 | 狀態 | 證據 |
|---|---|---|
handler 名稱 firmware_upgrade(cmd dispatch) |
✅ | 1942 行 main loop |
| stage 命名與 TDD §4.3 / Design 一致 | ✅ | _FW_STAGE_PERCENT 1218-1225 |
| reason enum 與 TDD §3.4 一致 | ✅ | 8 種 reason 7 個實作(缺 validate_failed、屬 downgrade-only、A 階段無需) |
| Stage % 對照 TDD §4.3(5/20/50/90/100/-1) | ✅ | 1218-1225 |
| Timeout 常數 60s/200s 對齊 AC-FW-1.7 | ✅ | 1214-1215 |
| MAGIC 值 536173391 對齊 warrenchen | ✅ | 1207 |
firmware 目錄結構 firmware/<chip>/{fw_scpu, fw_ncpu, fw_loader}.bin |
✅ | 已新增 KL520/fw_loader.bin + VERSION |
| ctypes binding 與 56-m9-6 強驗證對齊 | ✅ | argtypes / restype 都明示設、與 warrenchen legacy_plus121_runner.py 一致 |
對 M9-2(Go driver UpgradeFirmware() + firmware/service.go)的影響評估
| M9-2 依賴的 bridge.py 介面 | 已就緒 | 備註 |
|---|---|---|
cmd firmware_upgrade 接受 {port, chip} |
✅ | |
成功回 {status, before_firmware, after_firmware, method, duration_ms} |
✅ | |
失敗回 {error, stage, reason, raw_error} |
✅ | |
stderr push firmware_progress JSON event line |
✅ | |
stderr push shutdown_rejected event line |
✅ | |
| 行為穩定性 | 建議先修 M1 + M2 | M1(class 順序)不致命但 readability、M2(needs_loader 邏輯重構)讓 M9-2 寫 driver tests 時更容易理解、避免在 driver 端複製混淆 |
結論
審查結果
通過 with Major fixes — 阻擋 M9-2 啟動直到 M1 + M2 修復。
- ✅ 程式碼品質高、TDD §6.1 規格對齊度 98%
- ✅ 27 個 unit tests 涵蓋成功 + 失敗 + timeout + sigterm 主要場景
- ✅ ctypes 走法與 warrenchen reference 對齊、56-m9-6 強驗證結論落地
- ⚠️ 2 個 Major(class 順序 + needs_loader 邏輯)建議修完再啟 M9-2
- ⚠️ 4 個 Minor(雙重 chip allow-list / libkplus fallback sort / double-disconnect / 全域變數冗餘)建議跟 M9-2 整合時順手修
- 💡 4 個 Suggestion(拆函式 / poll 用 backoff / KDP 字串 match 改正則 / 補測試)可後續迭代
是否阻擋 M9-2 啟動
建議:阻擋、要求 backend 先修 Major M1 + M2、預估 0.2 人天內可完成。
理由:M9-2 寫 Go driver 時會基於 bridge.py 行為設計 retry / progress parser、若 bridge.py M2 的 needs_loader 控制流不清、driver 端容易誤判 stage 完成度。class 順序(M1)雖不致命但 fix 成本 5 分鐘、值得一起修。
是否需升級給 Security Auditor
否。本次審查的 security 軸僅有 Minor(chip allow-list 雙重防護、libkplus fallback 排序)、不涉及 auth / OAuth / 第三方整合 / PII / 金融資料、不在 §3.4 升級情境清單。
建議 Orchestrator 派 backend 修以下項目
優先級(高 → 低):
- M1:移
_FwError/_FwTimeoutError/_fw_handle_failure到handle_firmware_upgrade之前(檔案 1476 行附近) - M2:抽
should_run_loader_stagebool、重構 1596-1655 行的 nested 邏輯 - m1:在
_resolve_firmware_paths_full()內部加雙重 allow-list 防護 - m3:解決 finally double-disconnect 問題(單一 owner 原則)
- m4:砍
_firmware_upgrade_start_ts全域變數 - s4:補 4 個欠缺的 test case(loading-stage disconnect / loading-stage connect_failed / 完整失敗欄位 schema / ctypes binding)
Verification(Reviewer 自評)
| 項目 | 狀態 | Evidence |
|---|---|---|
| R-A1:5 軸 + 測試軸全跑過 | ✅ | Correctness(M1-M3 + m3 + s3)/ Readability(s1)/ Architecture(m4)/ Security(5 重點逐項評 + m1 m2)/ Performance(s2)/ Test(27 tests + s4 缺漏)—全部有實質判斷、無單軸用「OK」結案 |
| R-A2:文件符合性 checklist 完整 | ✅ | TDD §6.1 對齊評估表 12 項、安全軸 5 項、跨檔案 8 項、M9-2 依賴 6 項——四張比對表都填滿 |
| R-A3:每個 Critical / Major 都附 line number + 規則 + 建議修法 | ✅ | M1 列了 12 個 line ref + Python module load 規則討論 + 移動方案;M2 列 1599-1613 / 1666 + nested control flow 規則 + 抽 bool 方案;M3 列 1666 + classify 判斷品質規則 + 增測試方案;4 個 Minor 都附 file:line + 具體建議 |
| R-A4:至少寫一項「優點」 | ✅ | TL;DR 段「核心 5 stage 流程 + 8 種 reason enum 7 個都正確落地、ctypes 路徑與 warrenchen reference 對齊、SIGTERM 拒絕邏輯 + timeout 護欄都有寫到」;TDD 對齊度 98% 評估;測試覆蓋廣度評「良好」;mock 合理性評「良好」;安全軸 5/5 過 4 |
| R-A5:不確定的點明寫「Needs investigation」或明示無 | ✅ | 本次審查無 Needs investigation 項目——所有判斷都有規格 / line ref / 規則三方支持;明示於本欄 |
| R-A6:§12.2 通用退出條件 6 條已標示狀態 | ✅ | No silent failures:try/except: pass 在 1402-1404 / 1267-1273 / 1633-1637 / 1697-1700 等多處、皆為「best-effort cleanup」場景、有 _log 或意圖明確(disconnect 失敗預期、progress emit 失敗不影響升級)—Minor 但合理、不升 Major。No dead code:未發現。No hardcoded secrets:未發現(MAGIC 是公開常數 not secret)。No unsafe HTML/SQL:N/A(不適用 bridge.py)。Doc 同步:N/A(bridge.py 程式碼註解詳細、TDD 已寫好)。Working tree clean:未檢查 git status、reviewer 不動 source、不影響審查結論 |
| Verification 條件 | 結果 |
|---|---|
| 是否真的讀完整份 source(不是只看 diff) | ✅ 讀了 1200-1900 主體 + 1-200 + 200-300(context)+ 整份 test 檔 |
| 規格對照是否完整 | ✅ TDD §3.4 / §4.2 / §4.3 / §6.1 / §8.6 / AC-FW-1.6 / 1.7 / 1.9 全部對照 |
| 5 軸是否真的都過 | ✅ 全跑、各軸都有具體 finding 或明示無發現 |
完成回報:
- 報告路徑:
.autoflow/05-implementation/review/m9-1-bridge-firmware-upgrade-review.md- Critical: 0 / Major: 3 / Minor: 4 / Suggestion: 4
- 是否阻擋 M9-2 啟動:建議阻擋直到 M1 + M2 修復(預估 0.2 人天)
- 是否升級給 Security Auditor:否(安全軸僅有 Minor、不在升級情境清單)