Repository navigation
Fix cloud backup restore and recovery coverage - #886
Open
praveenperera wants to merge 66 commits into
Open
praveenperera wants to merge 66 commits into
praveenperera wants to merge 66 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: bitcoinppl/cove/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
praveenperera
force-pushed
the
cloud-backup-regression-fixes
branch
from
September 18, 2026 16:16
b866a1b to
c157895
Compare
praveenperera
marked this pull request as ready for review
September 18, 2026 16:16
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c157895d5d
ℹ️ 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".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
Author
|
@coderabbitai review |
Member
Author
|
@greptileai review |
|
praveenperera
force-pushed
the
cloud-backup-regression-fixes
branch
from
September 30, 2026 04:54
303171a to
17de442
Compare
Increments Android `versionCode` from 39 to 40 and iOS `CURRENT_PROJECT_VERSION` from 116 to 117 across project targets/configurations. This advances internal build metadata for the next release while leaving the app version name unchanged (`1.4.0`).
Hot wallets with a confirmed iCloud recovery copy no longer block cloud backup enable as unverified.
Bind each cloud-only restore/delete confirmation dialog to its row so SwiftUI presentation ownership stays with the presenter.
Record why verification is required so a wallet-set change can keep the prior proof for recovery coverage, while integrity loss and unconfirmed recovery keys still block coverage.
Unverified backups were left unchanged on startup integrity downgrade, so wallet-set coverage could stay valid. Re-mark verification required so recovery coverage drops until the user verifies again.
Update `CURRENT_PROJECT_VERSION` from 117 to 118 in the Xcode project so the app and related targets use the next iOS build number consistently across configurations.
Keep the fast local-snapshot Cloud Backup detail, then finish the same refresh with a provider listing so Restore All and other provider-gated actions unlock on iOS.
After a wipe PIN unlock, land on NewWallet.Select with an empty wallet list and navigation stack. Keep failed cleanup locked, clear cached session state before unlock, show the normal launch cover during wipe, and use fixed failure copy.
A full wipe replaces the database. Restore only the completed-setup flag so wiped devices keep finished onboarding state.
Associated-domain checks must see the final HTTP 200 body. Following redirects could accept a wrong file as valid.
Cloud backup passkeys must demand verification on create and assert. Also map unexpected credential types to a typed failure instead of a generic missing-credential error.
Native passkey failures need request-mode and presentation timing context so delayed or missing anchors are diagnosable.
Sensitive enable and verification actions must wait for the current prompt to finish dismissing. Staging the action until presenter readiness avoids reopening the old prompt.
Enable now inspects namespaces once for hints and matching, with bounded concurrent wrapper reads. Restore cancels cooperatively, keeps already matched namespaces, and no longer depends on a fixed onboarding timeout. Busy copy shows discovery while existing backups are checked.
Keep each process in its own cove-test temp root so parallel nextest runs do not share ~/.data, and sweep stale dirs after an hour.
Lock ManagerCache clearWalletManager so in-flight loads cancel and the related send flow manager is cleared too.
The testflight recipe always bumps the build number first. When the bump is already committed, as with build 119, there was no recipe that archives and uploads the current number. This wraps the existing xtask upload-testflight command so the CLI path can be used as is.
Keep LocalWalletConflict typed through the restore actor so an all-conflict run shows that local data was left unchanged instead of a generic cloud backup failure.
A bare platform cancelled code after Face ID does not prove the reader cancelled. State that the request did not complete, and log the discovery timing for later diagnosis.
After a coordinated delete succeeds, record process-local tombstones so stale provider snapshots cannot revive deleted paths for up to 60 seconds. Clear a file tombstone after a successful upload, and keep partial delete successes when cancellation wins.
set_authenticate_result now keeps a sticky default after queued results are consumed, so multi-call tests can reuse the configured authenticate response.
Provider redirects can differ from the requested path; tombstone both so deleted backups stay hidden from metadata listings.
Let restore tests cancel mid-save so keychain and persisted state stay consistent across one commit.
Cancellation between keychain writes and configured-state persist could leave a restored namespace half-activated.
Say "this device" instead of "iPhone" so the same restore conflict messages work on Android.
Replaced unconditional removal of the legacy `~/.data/test` directory with a guarded cleanup that only deletes directories positively identified as old Cove test DB storage (`cove_<alnum>.db` files only). Added focused tests covering safe deletion, unrelated/mixed contents, empty dirs, and nested dirs to prevent accidental data loss.
set_authenticate_result now only clears the queue and sets the sticky default, so tests can push one-shot failures without a duplicate queue entry of the same value.
Move cloud restore planning into its own module, replace RestoreEntry with Option writes, and drop unused cleanup warning tuples from the public restore result.
Pass keychain, passkey, namespace, state, and wallet ids as one RestoredNamespaceCommit, and share persist helpers between restore state writes.
Temp-dir test databases no longer leave ~./data/test behind, so the one-shot home cleanup and its tests are unused.
Move hot wallet metadata and keychain key suffixes into one place so backup and restore tests stop duplicating them and can assert by suffix name.
cargo test runs all tests in one process. Wipe-phase tests put the process-global coordinator in PreparingFullWipe. Persister tests that write through begin_persistence_operation then fail with CoordinatorBusy. Use in-memory storage for those persister tests. Keep persistent storage only for terminal wipe and deletion tests that already hold global_state_test_lock.
macOS accepted sockets inherit the listener's non-blocking mode. A read before the request bytes arrive would fail with WouldBlock.
Restore used to save the new keychain, persist configured state, then mark wallets dirty. A later write could leave the new keychain in place or wipe the previous one. Capture the prior keychain first, persist configured state and dirty wallet rows in one transaction, and restore the snapshot if that write fails.
When targeted authentication fails after the system prompt, matching already stops. Earlier successful namespace matches in that session stay restorable, same as native cancellation.
iOS and Android releases each copied the build number snapshot, bump, and rollback flow, and each validated key paths on its own. Both now use one bump_for_release helper and shared argument and file checks. The AASA check uses the existing no-redirect reqwest client instead of parsing curl output from stderr.
Review of this branch found duplicated retry paths, repeated keychain and database reads, and copied presentation wiring. Passkey cancellation is one non-optional flag, restore reads each keychain item once, forget_wallet uses one transaction, and shared helpers replace copies of timing logs, passkey hints, and settings updates. iOS moves the dismiss-then-dispatch handoff and prompt reconcile into one place, and Android drops dead exception arms and skips unused Rust reads on security toggles.
After a wipe, iOS and Android each re-read Rust state and decided what a wiped app looks like, and they had drifted: Android used a hardcoded fallback route and default auth values, and only iOS cleared the Cloud Backup enable completion. The wipe call now returns a FullWipeCompletion with the committed route, settings, onboarding flag, and auth settings, which each platform applies before unlocking. Follow-up reconcile messages carry the same values, auth PIN updates carry their state, and Cloud Backup drops a stale enable completion when it becomes disabled.
Full wipe deleted the receive session without the session store lock. A concurrent load that upgrades a legacy session could then save the old private key after the delete, and the next receive would resume it after a successful wipe. The wipe now deletes through the locked store.
If manager cleanup threw while applying the wipe completion, the app still unlocked but could keep its old wallet list and route. The visible state is now replaced before cleanup runs, and only the cleanup is guarded so a failure there cannot strand the lock screen or leave pre-wipe state on screen.
App Store Connect can accept an upload even when xcodebuild then reports failure. Rolling the build number back in that case made the next upload reuse an accepted number. The rollback now covers only the build and archive; a failed upload keeps the number and points to upload-testflight to retry, matching Android.
Remove assertions and unit tests that only restated stable request fields, error mapping, or recovery rules already covered elsewhere.
Those clap flag checks only mirrored the command definitions and did not protect release behavior.
praveenperera
force-pushed
the
cloud-backup-regression-fixes
branch
from
October 1, 2026 18:17
63f5362 to
ae764c3
Compare
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.
Summary
Fix cloud backup regressions around restore, passkeys, iCloud metadata, and recovery coverage.
Cove treated backup coverage too loosely. Cancelled checks, wallet-set changes, and integrity downgrades could leave the UI thinking a wallet still had a confirmed cloud recovery copy. Restore also failed to handle leftover Keychain items, local Keychain conflicts, deleted iCloud paths, and passkey request errors after the system prompt.
This branch:
Also included on this branch:
Testing
Not re-run as part of opening this PR. The branch adds Rust and iOS tests for recovery coverage, passkey match and restore, presentation handoff, and TestFlight xtask composition.
Platform Coverage
Checklist