fix: readiness-review follow-ups (two high-severity regressions from today) - #399
Merged
Merged
Conversation
An adversarial review of the 1.5.1 candidate found two high-severity regressions in changes landed earlier today. #394 create-wizard auto-advance could skip the seed backup entirely. It fired on device state alone: 'ready' while the create spinner shows. Hot-swapping a second, already-initialized KeepKey mid-create therefore read as success — the wizard announced 'Wallet Created!', labelled the WRONG device, and dropped the user past the only screen that says WRITE DOWN EVERY WORD (init-label had no such reminder, and verify-seed is skippable twice). Now the advance requires evidence: a disconnect must have been observed since this create started, AND the ready device must be the same deviceId the create began on. The write-down warning is also repeated on init-label, so no path through creation ends without it. #396's 'Enable on device' understated what it does. Enabling the persistent AdvancedMode policy also switches off this app's own pre-signature outflow simulation and per-transaction consent for every future opaque swap (swap.ts gates that path on isAdvancedModeEnabled). The copy now says so. Its RPC also moves from a 60s timeout to none: the operation blocks on an on-device confirm plus a full scrambled-matrix PIN re-entry, and a timeout could reject while the device still enables the policy. Also: patch-electrobun.sh now EXITS 1 when the rpc.ts dispatcher pattern is missing instead of warning — a silent revert means every device failure hangs the renderer for the full RPC timeout, with a green build. Previous is hidden on init-label (its target bounces forward again). The dev load-device path resets the same guards as the real create. An empty packet.error renders 'RPC request failed' again instead of the literal '""'.
The docs site now carries a page per wizard screen, each quoting the screen's copy verbatim so it is recognisable on arrival. Wire them up: - shared/docs-links.ts — one table of canonical slugs, the contract with the docs repo. No inline URL literals anywhere else. - DocsLink — one component so every screen links identically (wording, styling, new-tab safety attrs, click-through suppression). - All six tutorial cards carry a helpUrl, rendered beside the step counter. - Wizard footer shows the article for the current step (init-choose, create-briefing, init-progress, init-label, verify-seed). Screens with no dedicated page show nothing rather than dumping the user on a hub. - initChoose 'Learn more' repointed from the unverified keepkey.com/blog/why_does_keepkey_only_generate_12_words_ URL to the seed-length-and-entropy anchor, which is the same argument on a page we control.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An adversarial review of the 1.5.1 candidate found two high-severity regressions in changes landed earlier today, plus four smaller ones. All fixed here.
1. #394 auto-advance could skip the seed backup (HIGH)
The create-wizard advance fired on device state alone —
readywhile the create spinner is showing — with no evidence that a disconnect ever happened and no check that it is the same device.Repro: start Create on the OOB device, unplug it, plug in a second already-initialized KeepKey. Detach clears
setupInProgress, the new device pairs, PIN unlock →ready→ the wizard announces 'Wallet Created!', applies the label to the wrong device, offers verify-seed against the old seed, and finishes with confetti. No wallet was created, and the user never saw the backup screen — the 'WRITE DOWN EVERY WORD' panel exists only on init-progress, and verify-seed is skippable twice.Fix: the advance now requires (a) a
disconnected/connected_unpairedstate observed since this create started, and (b)deviceIdequal to the one the create began on. The write-down warning is additionally repeated on init-label, so no path through creation ends without it having been said.2. #396 'Enable on device' understated its blast radius (HIGH)
Enabling the persistent AdvancedMode policy also disables this app's own pre-signature outflow simulation and per-transaction consent for every future opaque swap (
swap.tsgates that path onisAdvancedModeEnabled). Offered at the moment of maximum pressure — a swap that just failed — in a flow previously designed as one-shot. The copy now states it plainly.Its RPC also moves 60s → no timeout: the call blocks on an on-device confirm plus a full scrambled-matrix PIN re-entry, and a timeout could reject while the device still completes the write — leaving the policy ON while the UI says it failed.
3. patch-electrobun.sh failed soft (MEDIUM)
On a pattern miss it printed a warning and exited 0, so an electrobun bump would silently revert the device-failure fix with a green build — symptom: every device error hangs the renderer for the full RPC timeout. Now
exit 1with an explanation.4-6. Smaller
init-label(its target bounces forward again — a no-op)packet.errorrenders 'RPC request failed' again instead of the literal""Verification
tsc differential vs develop: zero new errors.
make test-unit: 343 pass.