fix: stop stale device records from shadowing desktop BLE connectId resolution(OK-60878) - #12972
fix: stop stale device records from shadowing desktop BLE connectId resolution(OK-60878)#12972originalix wants to merge 2 commits into
Conversation
|
@codex review |
|
@codex security review |
|
@cursoragent review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
PR #12972 代码审查报告审查概要
评分
PR 评论分析
发现的问题[🟢 低] [🔵 High] 新增失败路径使用
|
| 优先级 | 置信度 | 文件 | 类型 | 描述 | Auto-fix |
|---|---|---|---|---|---|
| 🟢 低 | 🔵 High | ServiceHardware.ts:4501 | 规范 | catch 改用 defaultLogger.hardware.sdkLog |
✅ |
| 🟢 低 | 🟠 Medium | LocalDbBase.ts:5544 | 规范 | 补写路径 LocalDb 集成测试 | — |
测试建议
- Desktop BLE：已有钱包的设备 wipe 后经桌面蓝牙重新创建，批量派生不应再反复弹出 pairing dialog。
- 已中毒安装：第一次成功 repair 后循环应停止；
applySettingsToDevice/getDeviceLabel不再命中 stale 记录。 - 死钱包 fail-closed：旧钱包关联 Device 的
connectId被置空后应跳过解析或明确失败，而不是静默打到新种子设备。若 UI 拿「当前会话 serial」+ 旧钱包featuresDeviceId调用，identity miss 后会 fallback 到 live 记录——这是有意保留的 fallback，应用「设备已重置」UX 跟进。 - 回归：传入 stale
featuresDeviceId时，connectId-only fallback 仍能命中已绑定 BLE 的记录。
结论: 根因判断正确，读/写谓词对齐 + 验证后自愈是对症的，没有合入阻断项。完整报告已发到 PR #12972。
PR #12972 代码审查报告审查概要
评分
PR 评论分析
发现的问题[🟢 低] [🔵 High] 新增失败路径使用
|
| 优先级 | 置信度 | 文件 | 类型 | 描述 | Auto-fix |
|---|---|---|---|---|---|
| 🟢 低 | 🔵 High | ServiceHardware.ts:4501 | 规范 | catch 改用 defaultLogger.hardware.sdkLog |
✅ |
| 🟢 低 | 🟠 Medium | LocalDbBase.ts:5544 | 规范 | 补写路径 LocalDb 集成测试 | — |
测试建议
- 主修复路径（Desktop BLE）: 已有钱包的设备 wipe 后经桌面蓝牙重新创建钱包；批量派生账户不应再反复弹出 pairing dialog；sdkLog 出现
cleanStaleDeviceConnectIdAliases。 - 已中毒安装自愈: 当前已陷入 pairing 循环的库，第一次成功 repair 后循环应停止；connectId-only 调用方（
applySettingsToDevice/getDeviceLabel）不再命中 stale 记录。 - 死钱包 fail-closed: 旧钱包走
getWalletDeviceHwInfo时，关联 Device 的connectId已被置空，应跳过解析或明确失败，而不是静默打到新种子设备。若 UI 拿「当前会话 serial」+ 旧钱包featuresDeviceId调用getCompatibleConnectId，identity 查询 miss 后会 fallback 到 live 记录——这是本 PR 有意保留的 fallback，合入后应用「设备已重置」UX 跟进。 - 回归: 传入 stale
featuresDeviceId时，connectId-only fallback 仍能命中已绑定 BLE 的记录（现有 native/desktop 用例已覆盖）。 - Trezor desktop BLE: vendor 隔离应保证 OneKey 清理不会碰到 Trezor 记录；Trezor RPA
bleConnectId不与 OneKey serial 碰撞。
结论: 根因判断正确，读/写谓词对齐 + 验证后自愈是对症的，没有合入阻断项。上面两条都是 follow-up 级别。
— Auto-review by Cursor agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b120b15e50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
originalix
changed the title
2 participants
OK-60878
Summary
connectId+featuresDeviceId) over the connectId-only lookup ingetCompatibleConnectId, keeping the connectId-only match as a strict fallbacklocalDb.cleanStaleDeviceConnectIdAliasesand invoke it (best-effort) after every identity-verified desktop BLE bind — both the pairing-repair flow and the silent-bind flow — so stale sibling records lose their colliding connect-id aliases and existing poisoned databases self-healIntent & Context
A production v6.5.2 desktop (macOS) user wiped their OneKey Pro and re-onboarded it via desktop Bluetooth. Afterwards every hardware call — device details refresh, the batch account derivation right after wallet creation, settings reads — re-opened the "Pairing with your device…" dialog in an endless loop, even though each pairing repair reported success. Production logs showed
serviceHardware.repairBleConnectIdWithProgressbeing called and resolving successfully every ~10 seconds while the dialog kept coming back, with the NobleBLE link itself healthy the whole time.Root Cause
A device wipe changes the raw
deviceId, so wallet creation correctly produces a NEW Device record (getExistingDevicemust not silently re-associate a new seed onto an old wallet). But the pre-wipe record survives, and both records share the same serial-basedconnectId(the USB serial, e.g.PRB09B0058A):getCompatibleConnectIdresolved the device with a connectId-onlygetDeviceByQuery.getAllDevices()is unordered (dbDeviceId is a random UUID), and on the affected machine the stale pre-wipe record won the lookup. It has nobleConnectId, so the pairing dialog opened.connectId+featuresDeviceId, so it verified the live device and wrote the freshly boundbleConnectIdto the NEW record.connectId.deviceIdno longer matches the wiped device, so the repair's identity verification rejected with DeviceNotFound.Diagnosed from production logs by correlating the repair call/resolve cadence with the device identity change (
device_iddiffers across sessions while the serial stays the same), then verified line-by-line against the code.Design Decisions
getCompatibleConnectIdnow tries{connectId, featuresDeviceId}first and falls back to the connectId-only match. The original guarantee — stale device info must never veto a valid transport-ID match — is preserved because the fallback keeps the legacy behavior whenever the combined query misses.deviceIdprovably differs. Affected installs heal on the first successful bind, which also fixes callers that pass only a connectId (e.g.applySettingsToDevice,getDeviceLabel).deviceId; records without an identity are never treated as stale. Only colliding fields are cleared:connectIdto''(required column) andusbConnectId/bleConnectIdtoundefined. No schema change, noLOCAL_DB_VERSIONbump.getPersistedDesktopBleConnectIdis deliberately untouched — it protects against genuine serial-into-bleConnectId poisoning.cleanStaleDeviceConnectIdAliasesSafe): the bind already succeeded, so cleanup failures are logged and never surfaced.Changes Detail
packages/kit-bg/src/services/ServiceHardware/ServiceHardware.ts— identity-first lookup order ingetCompatibleConnectId; new privatecleanStaleDeviceConnectIdAliasesSafewired into therepairBleConnectIdWithProgressandsilentlyBindLiveDesktopBleConnectIdsuccess paths, with an sdkLog entry when records are cleaned.packages/kit-bg/src/dbs/local/LocalDbBase.ts— exported pure helperscollectDeviceConnectIdAliases/isStaleDeviceConnectIdAliasRecordand thecleanStaleDeviceConnectIdAliasesmethod (vendor-scoped, transaction-based, returns cleaned ids).packages/kit-bg/src/dbs/local/LocalDbBase.deviceConnectIdAliases.test.ts— new predicate unit suite (9 cases) covering every guard branch.packages/kit-bg/src/services/ServiceHardware/ServiceHardware.getCompatibleConnectId.test.ts— regression test for the duplicate-record pairing loop, a repair-flow cleanup wiring test, cleanup assertion added to the silent-bind success test; five existing tests migrated from sequence mocks to predicate-honoring mocks so they genuinely exercise the "stale identity does not veto the connectId match" fallback.Risk Assessment
getCompatibleConnectIdsits on every hardware call path — mitigated by the strict fallback (byte-for-byte legacy behavior whenever the combined query misses) and 435 passing tests across 26 hardware-related suites. Alias cleanup writes to sibling records — gated on a just-verified live identity plus a persisted-and-differentdeviceId, and it never breaks the bind on failure.Test plan
yarn jeston all ServiceHardware suites + the new predicate suite + related ServiceAccount/ServiceFirmwareUpdate/ServiceThirdPartyHardware suites — 26 suites, 435 tests passyarn agent:check --profile commitpassescleanStaleDeviceConnectIdAliases)