feat(mobile): add account persistence safeguards - #5083
Merged
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Files Reviewed (9 files)
Previous Review Summaries (6 snapshots, latest commit 0934214)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0934214)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryIncremental review of Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (35 files)
Fix these issues in Kilo Cloud Previous review (commit 86ea9fb)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryIncremental review of Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (39 files)
Fix these issues in Kilo Cloud Previous review (commit f9d2f92)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the two Files Reviewed (17 files)
Previous review (commit 73a3e16)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit refactors without regressions: credential/refresh rotation moves verbatim into a React-free Files Reviewed (26 files)
Previous review (commit 7b80e1f)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit fences the sign-out teardown window end to end — a teardown guard closes the refresh/cold-read resurrection race, the refresh fetch is bounded at the 15 s control-plane deadline, the remote-spawn draft marker arms only after admission, and draft/identity-hint writes no longer throw or leak rejections — resolving all six previous findings with no new issues in the 15 changed files. Files Reviewed (15 files)
Previous review (commit c2460d0)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryA post-sign-out race in the mobile auth layer can silently resurrect a revoked session: Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (76 files)
Reviewed by kimi-k3 · Input: 71.1K · Output: 4.4K · Cached: 219.6K Review guidance: REVIEW.md from base branch |
Plain SQLite ignores an unrecognized pragma without an error, so a successful `PRAGMA key` proved nothing: on a build without SQLCipher the database opened in plaintext and still passed the `sqlite_master` probe, putting drafts and the cached read set on disk unencrypted. Probe `PRAGMA cipher_version` before setting the key. It returns a row only when SQLCipher is linked in. A missing SQLCipher build throws `MissingSQLCipherError`, which skips delete-and-recreate recovery: a rebuild is the only fix, and wiping the file would destroy user drafts.
Clearing the memo on failure made every later call re-run the whole open: delete the database, regenerate the key, and report to Sentry. The read cache writes on each query settle, so a broken open burned the disk in a loop. Keep the memo on failure. Nothing an open failure hits is transient — a wrong key, a corrupt file, and a build without SQLCipher all need a relaunch or a rebuild.
Two cycles existed: auth-context -> exchange-legacy-token -> auth-context auth-context -> logout-cleanup -> trpc -> auth-context Both survived only because every use sat inside a function body. A future top-level read across either one fails at runtime, and typecheck does not catch it. Move the credential write queue and the refresh rotation — no React dependency — into `lib/auth/credentials.ts`. `trpc.ts` and `exchange-legacy-token.ts` now import that instead of the provider.
The deferred-destroy timer existed only to survive a StrictMode double-mount. The app mounts no StrictMode — `rg StrictMode apps/mobile` is empty — so the hook guarded a case that cannot happen, and carried the same abandoned-render residual as the plain ref it replaced. Restore the previous effect cleanup in the session provider and delete the hook with its test. The `retain()` change in the user web connection provider stays: that one fixes a real teardown.
Sign-out still revokes the current device session. It just no longer records a failed revoke for later retry. The device destroys the refresh token locally at sign-out, and the access token expires within 1 h, so an orphaned session row is already dead — it only lingers in the session list until then. Retrying it cost a tombstone field, a claim decoder, and half the reconciliation module. The push unregister keeps its retry: an unregistered device keeps receiving notifications for the signed-out account, which the user sees. Deletes `device-session-claim.ts` (the hand-rolled JWT payload decoder existed only to name the session for the retry) and its test.
`publishActiveTokenExpiry` already refuses to write when the held token or the epoch differs from the snapshot. The generation counter added a third comparison that can only differ when the same token was re-published at the same epoch — and both writers then store the same value, so the guard changed nothing.
`useNewSessionDraft` wrapped `useFencedDraftLoad` with one constant, and `resolveRestoredNewSessionPrompt` was a three-line exported function with one caller. Both go to their call site.
TypeScript already enforces `string` on every internal caller, so the `typeof` checks were dead. The empty-string checks stay and are now labelled: an empty scope silently widens `clearScopePrefix` to every scope in the database.
`CURRENT_DEVICE_BADGE` and `NO_CURRENT_DEVICE_NOTE` were exported constants read once each, in JSX. `UNKNOWN_DEVICE_LABEL` stays but is now module-local: its three uses are all inside `device-sessions.ts`. Also drops the test that asserted a constant equals its own literal.
The helper mixed two object members with one bare string, so callers
compared `outcome === 'not_found'` against `{ outcome: 'revoked' }`. Make
it a plain string union and let each procedure wrap its own result. The
wire shape is unchanged: both still return `{ outcome }`.
…sistence-089a # Conflicts: # apps/web/src/lib/trpc/init.ts # apps/web/src/lib/user/server.test.ts
eshurakov
approved these changes
Aug 17, 2026
jeanduplessis
approved these changes
Aug 17, 2026
Remove duplicated and speculative code in the account-persistence layer with no behavior change. Auth: - Replace the hand-rolled credential mutex with the existing chainSave helper. - Delete the invalidateRefreshSession and persistSignInCredentials wrappers. - Validate the logout tombstone with zod instead of a hand-written type guard. - Delete the ActiveTokenState type and the duplicate teardown-flag reads. - Add setAccountMetadata and use it at the four sites that inlined the wrapper. Persistence: - Import encrypted-kv directly and delete the key-value injection layer. - Delete the unread bytes column, its SQL, and scopeBytes. - Reuse utf8ByteLength from lib/utf8-utils. - Merge the two draft Sentry reporters and inline single-caller predicates. - Spread the base persister instead of forwarding two methods by hand. Features: - Extract useDraftFlushOnBackground for three identical AppState effects. - Move the draft hooks out of use-new-session-creator into lib/persist. - Delete the serializeConsentWrite wrapper and call chainSave directly. - Derive getDevicePushTokenOutcome from getDevicePushToken. Device sessions: - Partition instead of sorting on one boolean, and index the toast call. - Drop the unread kind field, two unused exports, and a single-use type. - Inline createTestTRPCContext into its only caller. Tests keep every assertion; the deleted cases covered deleted code.
Make Drizzle the only way the app runs SQL against expo-sqlite, and let Drizzle own the schema. - Define the key-value table in src/lib/persist/schema.ts and generate the initial migration under apps/mobile/drizzle/. - Replace every statement in encrypted-kv with the Drizzle query builder, and replace the inline CREATE TABLE with a migration run. - Keep the connection lifecycle on expo-sqlite, which Drizzle does not cover. Set PRAGMA key with execSync on the raw handle, because SQLCipher needs the key before the schema can be read and a prepared statement reads the schema. - Prove SQLCipher is linked with PRAGMA cipher_version before any key is set. - Re-run the migrations after delete-and-recreate recovery. - Add the babel inline-import plugin and the metro sql resolver the generated migration bundle needs. The open path is now synchronous, so statements block the JS thread. The exported API stays async and its callers are unchanged. A native dev-client build must still confirm that PRAGMA key survives on real SQLCipher, that the migration applies on a device file, and that the synchronous I/O does not stutter the UI.
- Clear the read cache by scope prefix, so a SCHEMA_VERSION bump no longer leaves the previous scope on the device forever. Sign-out cleared only the current version, which left up to 2 MB of the user's data after sign-out. - Prove the prefix delete keeps another user's rows and the drafts. - Delete isHardDeniedPath. It runs before the allowlist, and none of the four allowlisted paths can match it, so it cannot change an outcome. Its cheap denial assertions move to the allowlist test. - Drop the persister buster. The scope already carries the schema version, so an older blob lives at a scope the restore never reads.
The startup prefetch fires getMe, which refreshes an access token inside the refresh margin and writes a new auth-token with no React setter. The bootstrap then saw a changed stored token and returned without publishing anything, so a user with valid credentials reached the login screen and only a relaunch recovered. - Publish the winning token instead of returning empty-handed. The epoch fence still lets sign-out win, proved by a new test. - Replace deleteTombstoneIfUnchanged with a plain delete. SecureStore has no compare-and-delete, so the re-read and deep compare could not close the window they guarded. - Delete the tombstone when it belongs to another user. The previous user's push token can never be unregistered without their auth, so the record only kept a user id and a push token in a keychain that survives app deletion.
- Render the composer without waiting for user.getMe. The gate made the new session screen and the session screen refuse all typing on a cold start without a cached identity. The draft generation fence and the composer key already prevent a cross-account draft. - Apply a late draft only while the input is untouched, so text typed first survives. - Key the composer on an account epoch that bumps only between two known user ids, so resolving identity no longer remounts and discards typed text. - Keep the valid restored review items and drop only the invalid ones. One unrecognized row used to invalidate the whole array, which lost every queued review comment. - Lowercase the owner and the repo in the review draft key, so one pull request no longer splits into two queues by route casing. - Compute the composer render gate and the bottom bar from one value, so the bar no longer claims the composer's space before it exists. Text typed before identity resolves persists on the next keystroke after it arrives. Saving on the transition risks writing one account's text into another's scope.
- Return one success result from both revoke procedures. Nothing read the difference between revoked, already revoked, and not found: the only caller ignores the return value, and every case showed one toast and a refetch. A future additive outcome would also have shown a false error on shipped builds. - Keep one conditional update filtered on ownership and on an active row, and keep the tests that prove a caller cannot revoke a session they do not own. - Delete mapRevokeOutcome on the client. One success toast plus the standard error toast is shorter than the helper. - Say what the screen can do. Only Kilo app sign-ins create a session row, so editor and CLI sign-ins are not listed. Revocation stops refresh, and no auth path reads revoked_at, so a revoked device keeps API access until its access token expires after one hour.
The module flag in read-cache and the React state in auth-context carried the same fact to two consumers of one event, and three call sites kept them in sync by hand. Any new sign-out path could set one and forget the other. - Add src/lib/auth/sign-out-state.ts as the single source. It lives under auth because the fact is auth's, and it imports nothing, so there is no cycle. - Read it in React through useSyncExternalStore instead of mirroring it in state, and delete the two setters and the useState. - Leave the auth epoch, signOutTeardownActive, and isSignedOutReference alone. They gate different windows. The write-path fence still runs last, immediately before the key-value write.
pnpm matches minimumReleaseAgeExclude by package name, with no version bound, so each of the nine entries permanently exempted every future release of that package from the supply-chain maturity gate. The comment justified one urgent SDK 57 patch bump, not a standing exemption. The pinned versions have now aged past the gate: removing all nine installs clean and leaves the lockfile unchanged. A later bump to a fresh patch will wait for the gate, which is what the gate is for.
The delete, the key regeneration, and the recovery reopen sat outside the inner try, so a failure in any of them escaped without a Sentry report. The open memo keeps a failure, so one silent escape rejected every drafts and read-cache caller for the rest of the install with nothing in Sentry. Move the whole recovery inside the try and close the reopened handle only when it exists. The PRAGMA-key seam now reports once, like the probe seam.
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
3 participants
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.
Users can sign out this device, review active device sessions, keep encrypted read data, and restore drafts after a process restart.
Product behavior now revokes the current device session before local sign-out, retries failed cleanup, isolates cached data by account, and preserves high-intent drafts until completion.
The mobile app uses one auth epoch and one per-key serializer across credentials, SecureStore metadata, encrypted SQLCipher cache rows, drafts, and logout cleanup. Web tRPC procedures list and revoke owned device sessions. Provider resources now use reversible lifetimes.
Human Steps
Deploy web before the app build reaches users. Sign-out now calls
user.revokeCurrentDeviceSession, added in this PR. If the app build ships first, the mutation rejects against the old server, and a rejected revoke is never recorded or retried. The device session row then survives until its refresh token expires after 30 days and keeps showing in the user's session list.No manual migration, secret, or flag is required before merge.
The first native dev-client build after merge must include
expo-sqlitewith SQLCipher enabled. The completion gate records native build evidence before claiming P1-E-40a complete.The local key-value store now runs on Drizzle, which owns the schema through a generated migration under
apps/mobile/drizzle/. That native build must also confirm three things a unit test cannot reach: thatPRAGMA keysucceeds on the real SQLCipher build, that the migration applies on a device database file, and that the now-synchronous statements do not stutter the UI.SecureStore Key Table
auth-token,auth-refresh-token,auth-token-expires-at,auth-legacy-exchange-donesecretauth-attest-key-idsecretpersist-db-keysecretselected-organizationaccount-metadatawriteAccountMetadata/deleteAccountMetadataagent-session-filtersaccount-metadatawriteAccountMetadata/deleteAccountMetadataactive-user-idaccount-metadatawriteAccountMetadata/ sign-out deletelast-active-chat-instanceaccount-metadatawriteAccountMetadata/deleteAccountMetadatapr-review-recentsaccount-metadatawriteAccountMetadata/deleteAccountMetadatapr-review-viewedaccount-metadatawriteAccountMetadata/deleteAccountMetadataconsent-accepted-*account-metadata (persistent)chainSave; survives sign-outagent-model-preferencedevice-preferenceagent-reasoning-default-expandeddevice-preferencekeep-session-screen-ondevice-preferencestore-review-requested-atdevice-preferencewriteAccountMetadataserializer; existing retentionnotification-prompt-seendevice-preferencekiloclaw-ownedaccount-metadatalogout-cleanup-tombstonesafe-retrypush-unregister stateVisual Changes
Screenshots are not attached because the E2E bundle could not complete the device-session and draft flows.
E2E
E2E was partially verified. The repository flow initially expected four tabs, while this branch renders three; a workflow fix added compatible selectors. The corrected run verified login, the three-tab profile route, current-device sign-out, database session revocation with
revoked_reason = logout, and zero push-token rows. A takeover also opened the Device sessions screen and verified the current-device action and database cleanup. Device-session inventory with a second session, encrypted cache restore, durable drafts, pending review restore, account switching, and remote spawn draft clearing did not complete because the available seed and flow infrastructure could not safely create the required second session and scenario data.Deviations
The repository's Expo SDK patch alignment required
minimumReleaseAgeExcludeentries for the current SDK 57 patch train. No plaintext cache fallback exists. P1-E-40a remains gated on native SQLCipher build evidence until the dev-client build completes.