Repository navigation
Add native identity backup consent hooks - #1137
Conversation
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Codex <codex@openai.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Codex <codex@openai.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict: one major finding in this layer (inline), no other defects found. The consent read/lease logic and the config validation hold up.
The stack-wide scope concern (consent hooks with no production caller) is filed once on #1106.
Stack reviewed at #1137 9bac2d1 → #1106 417b30a → #1107 b3109cf → #1101 f3a5728 (base main 1ede3e3), with independent native, recovery and frontend lanes plus a cross-layer pass.
Not verified: live browser sign-in, KMS, installed-app secure-store behavior and runtime race ordering. Reviewers relied on existing CI and did not rerun suites locally.
Merge criteria: make a successful consent change crash-durable before #1106 relies on it.
| .write_all(&serde_json::to_vec(&record).map_err(|_| FAILED)?) | ||
| .map_err(|_| FAILED)?; | ||
| staged.as_file().sync_all().map_err(|_| FAILED)?; | ||
| staged.persist(path).map_err(|_| FAILED)?; |
There was a problem hiding this comment.
Major: a successful consent revocation isn't crash-durable. set syncs the staged file and then renames it over choice.json, but never fsyncs the parent directory. Suppose choice.json holds automatic: true and the user turns backup off. The command returns success. If power is lost before the rename's directory entry is durable, the old true record can come back. With #1106 on top, the next authenticated pass reads that stale authorization and uploads the sealed nsec after the user opted out.
Fix: after persist, sync the parent directory on Unix, as nsec_backup/persistence.rs already does after its own replace. Only report success after that, and cover the sync failure path with a test.
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: jm <jm@squareup.com> Co-authored-by: Codex <noreply@openai.com>
| staged | ||
| .write_all(&serde_json::to_vec(&record).map_err(|_| FAILED)?) | ||
| .map_err(|_| FAILED)?; | ||
| staged.as_file().sync_all().map_err(|_| FAILED)?; |
There was a problem hiding this comment.
🤖 [P2] Persist the directory update before acknowledging consent changes.
Replacing an existing opt-in with false syncs the temporary file but returns after rename without syncing its parent directory. Power loss can leave the old valid opt-in, allowing a later authenticated matching-key upload despite the acknowledged opt-out. Keep the exclusive lease through parent-directory sync, propagate sync failures, and cover that failure path. This corroborates the existing durability feedback; the crash scenario was source-traced, not experimentally reproduced.
| exportKey: () => invoke<string>("identity_export"), | ||
| /** Public consent only; reading or saving it does not start a backup. */ | ||
| getBackupChoice: () => invoke<BackupChoice>("nsec_backup_choice"), | ||
| /** Pass the key returned by getBackupChoice so a stale selection cannot affect another key. */ |
There was a problem hiding this comment.
🤖 The complete stack still has no production caller for these consent methods, so users cannot enable backup through the supplied UI. Searches of .github, scripts, justfile* and package.json also found no backup service URL/namespace configuration; this does not rule out external build configuration. Please wire the current consent consumer and intended build configuration, or explicitly document/approve this as staged infrastructure under the repository's current-caller requirement.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account. Follow-up at head 65e3ab75be46106ac043967624b9da9b1c8b57fd, base 1ede3e310350a27dfe29c52fb6e2956dea7df3e5.
No additional changes requested in this follow-up (9/10). The prior durability finding is addressed: after replacing choice.json, the Unix path syncs the containing directory before returning success, while retaining the exclusive lease. The regression covers directory-open failure after replacement, reports failure rather than durable success, and then retries successfully. No new defect found in the two-file repair.
The separate stack-wide consent-selection concern remains tracked on #1106; this review does not establish that end-to-end backup consent is complete. Public description, commit messages and changed source/configuration inspected; no attached images.
Source-only: no code, tests, builds or app executed; current CI not assessed. Crash/power-loss durability, installed-app storage, cross-process ordering and human acceptance remain unverified. No UI/focus transition is changed by this repair. COMMENT only, not approval.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Approved for staged infrastructure without a current consumer. The lock-contention hypothesis is a non-blocking follow-up; native acceptance and local test-failure attribution remain outstanding.
| .create(true) | ||
| .truncate(false) | ||
| .open(path.with_extension("lock")) | ||
| .map_err(|_| FAILED)?; |
There was a problem hiding this comment.
🤖 [P3, non-blocking hypothesis] Investigate transient lock contention around process spawning.
At stack tip d031907, intermittent native tests failed at consent/persistence lock acquisition, while 20 backup-only runs passed. A possible explanation is forked children retaining a lock file description until exec; this mechanism and its production impact are not proven. If confirmed, consent saves could report busy spuriously and backup passes could defer until another event. Reproduce the interaction before choosing bounded lock retry or another fix. The related persistence lock is in src-tauri/src/nsec_backup/persistence.rs:127 at that stack tip.
* origin/main: Add native Huddles with call controls and conversation panels (#489) fix(claude-code): keep sessions alive while Claude's background work runs (#1102) Rename Agent setup "Providers" to "Harness defaults" (#1348) feat(plugins): feature core plugins and branded catalog browsing (#1246) Improve AI defaults and add Goose configuration (#1266) feat(sidebar): filter channel sections by unread and activity (#1297) feat(plugins): organize settings with guides and focused controls (#1176) Unify Solo chat menus, rename workspaces, quieter membership notices (#1326) Add identity restore UI (#1101) Add native identity backup recovery (#1107) Back up desktop identities only with saved consent (#1106) Add native identity backup consent hooks (#1137) remove the Channel Usage plugin (#1335) fix(messages): retain image geometry across virtualized remounts (#1271) fix(channels): read metadata for a labeled channel whose roster came first (#1289) fix(channel-members): allow adding members to system channels (#1288) fix(unread): recover dropped thread roots for unread thread activity (#1291) test(voice-notes): fix the draft-card race that turned main red (#1315) ci: check dependency policy with cargo-deny (#1321) Signed-off-by: jtennant <jtennant@squareup.com> # Conflicts: # src/bundled/inbox/InboxDetail.test.tsx # src/bundled/inbox/InboxDetail.tsx
…x-session-preview-live * origin/jt/inbox-page-tabs: (25 commits) refactor(inbox): open conversations through conversation.ui.Tabs fix(inbox): retire a Huddle room's retained tabs and keep their focus test(inbox): stop depending on Channels having loaded a thread's channel fix(inbox): close an opened conversation once its room becomes a Huddle Add native Huddles with call controls and conversation panels (#489) fix(inbox): offer no Archive shortcut while Drafts is shown fix(inbox): keep a restored tab's row selected after leaving Inbox fix(claude-code): keep sessions alive while Claude's background work runs (#1102) Rename Agent setup "Providers" to "Harness defaults" (#1348) feat(plugins): feature core plugins and branded catalog browsing (#1246) Improve AI defaults and add Goose configuration (#1266) feat(sidebar): filter channel sections by unread and activity (#1297) feat(plugins): organize settings with guides and focused controls (#1176) Unify Solo chat menus, rename workspaces, quieter membership notices (#1326) Add identity restore UI (#1101) Add native identity backup recovery (#1107) Back up desktop identities only with saved consent (#1106) Add native identity backup consent hooks (#1137) remove the Channel Usage plugin (#1335) fix(messages): retain image geometry across virtualized remounts (#1271) ...
Prepare the native consent API for a later explicit automatic-backup selection, without changing onboarding, settings, or identity startup.
Public getBackupChoice() / setBackupChoice(pubkey, automatic) hooks persist a choice for the current key and trusted backup destination. Missing, corrupt, mismatched or self-managed choices cannot authorize uploads. Native read leases serialize admitted uploads against choice changes. Consent replacement syncs the file and, on Unix, its parent directory before reporting success. Plain sign-out retains the choice; erase removes it through the existing resumable wipe.
No uploader or copied HPKE code is introduced here. The next PR consumes the hooks.
Stack: #1137 consent hooks → #1106 gated uploader → #1107 recovery → #1101 restore UI.
Validation: seven focused configuration/consent tests and 51 erase tests passed on Blox, including fail-then-pass directory-sync regression evidence. Required local push hooks passed; five independent reviews found no remaining blockers. No browser journeys added or removed. Hosted CI is tracked in the PR checks. Native app, crash/power-loss execution and human acceptance remain pending. No real private keys were used.