feat(secure): migrate TokenStore to KSafe (Phase 1)#632
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds KSafe (catalog + dependency), registers named KSafe instances for tokens/prefs/announcements, implements a typed DataStore→KSafe migrator, and migrates multiple repositories (tokens, tweaks, proxy, mirror, announcements, device id, external import, slow-download) to KSafe with guarded one-time migrations; release notes and project version updated. ChangesKSafe migration and secure persistence
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR migrates all major persistent stores from plaintext
Confidence Score: 3/5The migration logic across most stores is careful and correct, but the proxy write path lost the atomicity guarantee that DataStore provided — a mid-write crash corrupts the stored config silently. The core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.kt — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App Launch / Singleton Init] --> B{KSafe marker set?}
B -- Yes --> C[Skip migration\nRead from KSafe directly]
B -- No --> D[Read legacy DataStore snapshot]
D --> E{Snapshot readable?}
E -- No --> F[Abort — retry next launch\nmarker NOT set]
E -- Yes --> G[Copy entries to KSafe\nper-entry success tracking]
G --> H{Any write failure?}
H -- Yes --> I[Delete only successfully\ncopied legacy keys\nmarker NOT set]
H -- No --> J[Delete all legacy keys]
J --> K{Delete succeeded?}
K -- No --> I
K -- Yes --> L[Set migration marker in KSafe]
L --> C
I --> M[Retry on next launch]
style F fill:#f9f,stroke:#c33
style I fill:#f9f,stroke:#c33
style M fill:#f9f,stroke:#c33
style L fill:#bfb,stroke:#393
style C fill:#bfb,stroke:#393
Reviews (6): Last reviewed commit: "chore: drop committed .DS_Store + ignore..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.kt`:
- Around line 186-190: The KSafe instance created in the single qualifier
named("tokens") uses fileName "ghs-tokens" which violates KSafe 2.0.0's filename
regexp; change the fileName in the KSafe constructor to use an underscore (e.g.,
"ghs_tokens") and update the same occurrence in the JVM platform module so both
KSafe constructors (the one in PlatformModule.android.kt and its JVM
counterpart) use a valid name matching [a-z][a-z0-9_]*.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt`:
- Around line 35-45: The save() and clear() paths run concurrently with the
background migrateIfNeeded() (launched from migrationScope) and bypass the
migrationLock, allowing races where legacy tokens overwrite or are left behind;
update save() and clear() to acquire the same migrationLock used by
migrateIfNeeded() (e.g., call migrationLock.withLock { ... } or otherwise
suspend until migration completes) around reads/writes (references:
migrationScope, migrateIfNeeded(), migrationLock, save(), clear(), ksafe.put,
tokenKey) so that normal writes are serialized with the migration gate.
- Around line 90-102: The migration currently removes the legacy key and sets
migrated = true even when
json.decodeFromString(GithubDeviceTokenSuccessDto.serializer(), legacyRaw) or
ksafe.put(tokenKey, parsed) fail; change the flow so you only remove
legacyDataStore.edit { it.remove(legacyKey) } and set migrated = true after a
successful parse and a successful ksafe.put; use runCatching around decode and
around ksafe.put and only proceed to edit/remove and set migrated when both
runCatching results are successful (i.e., parsed != null and the ksafe.put
runCatching completed without exception), leaving the legacy entry intact and
migrated false if either step fails so a retry can occur on next read.
- Around line 47-54: Replace the one-shot callbackFlow in
DefaultTokenStore.tokenFlow() with KSafe's reactive API: use
ksafe.getFlow(tokenKey, defaultValue = null, encrypted = true?) or
ksafe.getStateFlow(tokenKey, defaultValue = null, encrypted = true, scope =
coroutineScope) so the Flow emits on subsequent save() and clear() calls; update
tokenFlow() to return that Flow/StateFlow directly and ensure you pass the same
tokenKey and encryption flag used by save()/clear() and provide a scope if using
getStateFlow().
In `@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.kt`:
- Around line 99-100: The KSafe registration in PlatformModule.jvm.kt uses an
invalid fileName "ghs-tokens" for eu.anifantakis.lib.ksafe.KSafe; change the
fileName to match KSafe 2.0.0's required pattern ([a-z][a-z0-9_]*), e.g. use
"ghs_tokens" or "ghstokens", by updating the KSafe(...) invocation so the
fileName argument conforms to the allowed characters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baa96649-4462-4d79-b35e-0eaa16edc0db
📒 Files selected for processing (19)
core/data/build.gradle.ktscore/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/18.jsongradle/libs.versions.toml
…, Mirror, Announcements, DeviceIdentity, ExternalImport, SlowDownload)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/secure/DataStoreToKSafeMigrator.kt (1)
9-20: ⚡ Quick winRemove the new KDoc block to match repo convention.
This file adds a top-level KDoc block that isn’t required for a non-obvious invariant/workaround case.
As per coding guidelines "Do not add KDoc or inline comments unless explicitly requested; only add inline comments for non-obvious invariants, tricky concurrency, or workarounds".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/secure/DataStoreToKSafeMigrator.kt` around lines 9 - 20, Remove the top-level KDoc block added to DataStoreToKSafeMigrator.kt so the file follows repo convention; specifically delete the multi-line KDoc comment at the top of the file that describes the one-shot copier behavior (the block above the DataStoreToKSafeMigrator implementation), leaving only necessary code and preserving any non-obvious inline comments inside functions like migrate or related methods if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AnnouncementsCacheStoreImpl.kt`:
- Around line 52-58: The legacy cache is being cleared unconditionally even if
the ksafe.put(K_CACHED_PAYLOAD, legacy) write fails; change the flow so
legacyDataStore.edit {
remove(stringPreferencesKey("announcements_cached_payload")) } only runs after
runCatching { ksafe.put(K_CACHED_PAYLOAD, legacy) } succeeds — i.e., check the
result (or use onSuccess) of ksafe.put before calling legacyDataStore.edit, and
keep the existing logger.w on failure for the legacy clear branch; reference the
ksafe.put(K_CACHED_PAYLOAD, legacy) call and
legacyDataStore.edit/remove(stringPreferencesKey("announcements_cached_payload"))
to locate the code to adjust.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.kt`:
- Around line 36-40: resetDeviceId currently only writes the new DEVICE_ID_KEY
via ksafe.put and can leave the legacy "anonymous_device_id" key behind if reset
happens before getDeviceId; update resetDeviceId (inside the
deviceIdMutex.withLock block) to also remove the legacy key (e.g., call
ksafe.remove("anonymous_device_id") or equivalent) so the old plaintext ID is
cleared whenever a new device id is generated, ensuring both the new
DEVICE_ID_KEY is written and the legacy anonymous_device_id is deleted
atomically under the same mutex.
- Around line 54-57: The legacy deletion is happening unconditionally; change
the logic so that the legacyDataStore.edit call that removes
stringPreferencesKey("anonymous_device_id") runs only if the secure write via
ksafe.put(DEVICE_ID_KEY, legacy) succeeds; specifically, check the result of
runCatching { ksafe.put(DEVICE_ID_KEY, legacy) } (or use onSuccess/onFailure)
and invoke legacyDataStore.edit {
it.remove(stringPreferencesKey("anonymous_device_id")) } only in the success
branch to avoid losing the prior stable ID when KSafe write fails.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt`:
- Around line 485-488: The code currently calls runCatching {
ksafe.put(K_INITIAL_SCAN_AT, legacyValue) } and then unconditionally clears the
legacy flag via legacyDataStore.edit {
remove(longPreferencesKey("external_import_initial_scan_at")) }, which can drop
the migration source if the KSafe write failed; update
ExternalImportRepositoryImpl so the legacy preference is removed only when the
KSafe write succeeds — e.g., capture the result of runCatching for
ksafe.put(K_INITIAL_SCAN_AT, legacyValue) and in its onSuccess block call
legacyDataStore.edit {
it.remove(longPreferencesKey("external_import_initial_scan_at")) } (or check
isSuccess before calling legacyDataStore.edit) to ensure removal happens only
after successful ksafe.put.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.kt`:
- Around line 178-199: The migration currently ignores failures from each
ksafe.put call and then unconditionally clears legacyDataStore keys and sets
MIGRATION_MARKER, which can destroy data on partial failures; update the
migration in ProxyRepositoryImpl so you collect the results/errors from each
ksafe.put (do not swallow them with runCatching without checking), only proceed
to legacyDataStore.edit(...) and runCatching { ksafe.put(MIGRATION_MARKER, true)
} after all required ksafe.puts have succeeded, and if any put failed abort the
cleanup (propagate or log the error and leave legacy keys intact) so
credentials/config are not lost.
- Around line 35-37: The background migrator started in init
(migrationScope.launch { migrateIfNeeded() }) can finish after KSafe is accessed
by getProxyConfig() and setProxyConfig(), so gate all KSafe accesses until
migration completes: add a migration completion synchronization primitive (e.g.,
a CompletableDeferred or a Mutex+Boolean) that migrateIfNeeded() completes/sets
when done, then make getProxyConfig(), setProxyConfig(), and any other methods
that touch KSafe await that primitive before proceeding; update references in
ProxyRepositoryImpl (notably migrateIfNeeded, getProxyConfig, setProxyConfig and
the blocks around lines ~62-73, ~108-137, ~143-146) to call
ensureMigrationFinished()/awaitMigration() so reads/writes happen only after
migration finishes.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt`:
- Around line 220-253: Concurrent read-modify-write on preferences in
setLastSeenWhatsNewVersionCode, addAnnouncementDismissedId,
addAnnouncementAcknowledgedId, and setAnnouncementCategoryMuted can lose
updates; make these operations atomic by serializing them (either with ksafe's
atomic update API if available or with a repository-level Mutex). Wrap the
current get/put sequence for K_LAST_SEEN_WHATS_NEW_WHATS_NEW_VERSION_CODE,
K_ANNOUNCEMENTS_DISMISSED_IDS, K_ANNOUNCEMENTS_ACKNOWLEDGED_IDS, and
K_ANNOUNCEMENTS_MUTED_CATEGORIES in a single critical section (e.g., declare a
private val prefsMutex = Mutex() and use prefsMutex.withLock { val current =
ksafe.get(...); compute updated; ksafe.put(...) }) so reads and writes cannot
interleave and overwrite each other. Ensure all four methods use the same
mutex/atomic update to guarantee serialization.
- Around line 33-44: The init currently launches migrateDataStoreToKSafe in
migrationScope (migrationScope, migrateDataStoreToKSafe, legacyDataStore, ksafe,
MIGRATION_MARKER, legacyEntries) as a fire-and-forget task which allows
consumers to observe KSafe defaults before migration finishes; fix by blocking
reads until migration completes: either run the migration synchronously in init
(e.g., perform the migrateDataStoreToKSafe call on the calling coroutine/thread
instead of launching) or add a migrationReady signal (CompletableDeferred or
Deferred returned from migrationScope.launch/async) that all public read/write
entry points await before accessing ksafe, and set/complete that signal when
migrateDataStoreToKSafe returns/throws so no KSafe defaults are visible
pre-migration.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/secure/DataStoreToKSafeMigrator.kt`:
- Around line 30-33: The migrator currently writes ksafe.put(markerKey, true)
even when legacy.data.first() or subsequent copy operations fail; change the
logic so that markerKey is only set after the entire migration succeeds.
Specifically, in DataStoreToKSafeMigrator, stop calling ksafe.put(markerKey,
true) inside the failure branches around legacy.data.first() or
partial-runCatching blocks and instead perform the ksafe.put(markerKey, true)
only after the successful read-and-copy sequence completes (i.e., after the code
that iterates/copies the snapshot and returns success). Update all occurrences
mentioned (the runCatching around legacy.data.first(), the blocks at lines
~37-42 and ~52-53) to check runCatching.isSuccess or handle exceptions and only
write the marker on success.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/secure/DataStoreToKSafeMigrator.kt`:
- Around line 9-20: Remove the top-level KDoc block added to
DataStoreToKSafeMigrator.kt so the file follows repo convention; specifically
delete the multi-line KDoc comment at the top of the file that describes the
one-shot copier behavior (the block above the DataStoreToKSafeMigrator
implementation), leaving only necessary code and preserving any non-obvious
inline comments inside functions like migrate or related methods if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df09ed6c-8f52-4f08-9798-62f682eecfed
📒 Files selected for processing (25)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/download/SlowDownloadDetectorImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorPersistence.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AnnouncementsCacheStoreImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/secure/DataStoreToKSafeMigrator.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/18.json
💤 Files with no reviewable changes (1)
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorPersistence.kt
✅ Files skipped from review due to trivial changes (9)
- core/presentation/src/commonMain/composeResources/files/whatsnew/tr/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/bn/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/ru/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/ar/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/hi/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/ja/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/es/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/it/18.json
🚧 Files skipped from review as they are similar to previous changes (2)
- core/presentation/src/commonMain/composeResources/files/whatsnew/ko/18.json
- core/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/18.json
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
… legacy-clear, atomic RMW, never mark complete on failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt (1)
42-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep retrying the DataStore→KSafe migration until it actually completes.
This deferred is completed after the first attempt regardless of
migrateDataStoreToKSafe()outcome. That defeats the helper’s retry contract: after one transient failure, every getter/setter permanently falls back to KSafe defaults instead of retrying the legacy copy on the next access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt` around lines 42 - 59, The init block currently completes migrationDeferred unconditionally after the first migrateDataStoreToKSafe() attempt, which prevents retries on transient failures; change the logic so migrationDeferred.complete(Unit) is only called when migrateDataStoreToKSafe(...) succeeds (e.g., check runCatching result or use try/catch), and on failure do not complete the deferred so gatedGetFlow (and other awaiters) will keep waiting and cause subsequent accesses to re-run or retry the migration; locate the init coroutine and the migrateDataStoreToKSafe invocation plus migrationDeferred to implement this flow control (or add a retry loop with backoff that completes migrationDeferred only on successful migration).core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt (1)
37-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't open the token store after an unsuccessful migration attempt.
migrationDeferredis completed unconditionally, butmigrateIfNeeded()has several early-return failure paths. That means a transient legacy read/parse/write/cleanup failure permanently switches all later reads to KSafe-only, which can sign existing users out or leave the plaintext legacy token behind forever with no retry path.Also applies to: 81-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt` around lines 37 - 41, The init migration currently completes migrationDeferred unconditionally even if migrateIfNeeded() failed or returned early; change the logic so migrationDeferred.complete(Unit) is only called when migrateIfNeeded() actually succeeds. Replace the unconditional runCatching { migrateIfNeeded() }; migrationDeferred.complete(Unit) with a guarded flow that runs migrateIfNeeded() and on success calls migrationDeferred.complete(Unit), and on failure logs/handles the error (or completes the deferred exceptionally) but does NOT complete it normally so consumers don't treat an unsuccessful migration as done; apply the same guarded change to the other migration block (the migrationScope.launch usage around migrateIfNeeded() at lines ~81-106) referencing DefaultTokenStore, migrationScope.launch, migrateIfNeeded(), and migrationDeferred to locate the spots.core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.kt (1)
50-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark migration done while the plaintext legacy ID may still exist.
If KSafe already has a value, or if the
legacyDataStore.editcleanup fails, this method still flipsmigrated = true. After that, the oldanonymous_device_idcan stay on disk permanently after a failed cleanup or reset path.Suggested fix
private suspend fun migrateIfNeeded() { if (migrated) return - val existing = runCatching { ksafe.get(DEVICE_ID_KEY, "") }.getOrDefault("") - if (existing.isNotBlank()) { - migrated = true - return - } + val existing = runCatching { ksafe.get(DEVICE_ID_KEY, "") }.getOrElse { return } val legacy = runCatching { legacyDataStore.data.first()[stringPreferencesKey("anonymous_device_id")] }.getOrNull() + if (existing.isNotBlank() && legacy.isNullOrBlank()) { + migrated = true + return + } if (!legacy.isNullOrBlank()) { val putResult = runCatching { ksafe.put(DEVICE_ID_KEY, legacy) } if (putResult.isFailure) { // Keep legacy intact for next attempt. return } - runCatching { + val deleteResult = runCatching { legacyDataStore.edit { it.remove(stringPreferencesKey("anonymous_device_id")) } } + if (deleteResult.isFailure) return } migrated = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.kt` around lines 50 - 70, The migration currently sets migrated = true even when legacy cleanup fails, which can leave the plaintext anonymous_device_id on disk; update migrateIfNeeded so migrated is only set to true when either an existing KSafe value is present or when the legacy value was successfully moved AND the legacyDataStore.edit removal succeeded: inside migrateIfNeeded, keep the early-return for existing KSafe values, but for the legacy branch, only set migrated = true after runCatching { ksafe.put(DEVICE_ID_KEY, legacy) } is successful AND the subsequent runCatching { legacyDataStore.edit { it.remove(stringPreferencesKey("anonymous_device_id")) } } also succeeds; if either operation fails, return without flipping migrated so the cleanup can be retried later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt`:
- Around line 67-70: DefaultTokenStore.clear() currently swallows failures from
ksafe.delete(tokenKey) by wrapping it in runCatching; change it so deletion
failures are not ignored: after migrationDeferred.await() call
ksafe.delete(tokenKey) without runCatching so any exception propagates (or if
you want to attempt legacy cleanup too, catch the ksafe exception into a
variable, still call legacyDataStore.edit { it.remove(legacyKey) } (you may keep
that in runCatching), and then rethrow the original ksafe exception), ensuring
exceptions from ksafe.delete(tokenKey) surface to callers of
DefaultTokenStore.clear().
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.kt`:
- Around line 25-33: The getDeviceId() implementation currently treats any
ksafe.get failure as "missing" and may generate a new ID; change it so a read
error is not turned into a new ID: inside deviceIdMutex.withLock call, call
migrateIfNeeded(), then attempt ksafe.get(DEVICE_ID_KEY, "") but if that
operation fails propagate the exception (or retry) instead of using
getOrDefault(""); only when the read succeeds and returns blank should you
generate a new UUID (Uuid.random().toString()), persist it with
ksafe.put(DEVICE_ID_KEY, generated) and return it; in other words, replace the
runCatching(...).getOrDefault("") pattern with explicit success/failure handling
so failures from ksafe.get are not treated as missing.
---
Duplicate comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt`:
- Around line 37-41: The init migration currently completes migrationDeferred
unconditionally even if migrateIfNeeded() failed or returned early; change the
logic so migrationDeferred.complete(Unit) is only called when migrateIfNeeded()
actually succeeds. Replace the unconditional runCatching { migrateIfNeeded() };
migrationDeferred.complete(Unit) with a guarded flow that runs migrateIfNeeded()
and on success calls migrationDeferred.complete(Unit), and on failure
logs/handles the error (or completes the deferred exceptionally) but does NOT
complete it normally so consumers don't treat an unsuccessful migration as done;
apply the same guarded change to the other migration block (the
migrationScope.launch usage around migrateIfNeeded() at lines ~81-106)
referencing DefaultTokenStore, migrationScope.launch, migrateIfNeeded(), and
migrationDeferred to locate the spots.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.kt`:
- Around line 50-70: The migration currently sets migrated = true even when
legacy cleanup fails, which can leave the plaintext anonymous_device_id on disk;
update migrateIfNeeded so migrated is only set to true when either an existing
KSafe value is present or when the legacy value was successfully moved AND the
legacyDataStore.edit removal succeeded: inside migrateIfNeeded, keep the
early-return for existing KSafe values, but for the legacy branch, only set
migrated = true after runCatching { ksafe.put(DEVICE_ID_KEY, legacy) } is
successful AND the subsequent runCatching { legacyDataStore.edit {
it.remove(stringPreferencesKey("anonymous_device_id")) } } also succeeds; if
either operation fails, return without flipping migrated so the cleanup can be
retried later.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.kt`:
- Around line 42-59: The init block currently completes migrationDeferred
unconditionally after the first migrateDataStoreToKSafe() attempt, which
prevents retries on transient failures; change the logic so
migrationDeferred.complete(Unit) is only called when
migrateDataStoreToKSafe(...) succeeds (e.g., check runCatching result or use
try/catch), and on failure do not complete the deferred so gatedGetFlow (and
other awaiters) will keep waiting and cause subsequent accesses to re-run or
retry the migration; locate the init coroutine and the migrateDataStoreToKSafe
invocation plus migrationDeferred to implement this flow control (or add a retry
loop with backoff that completes migrationDeferred only on successful
migration).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdfe48e9-9cbb-4394-a0f4-706fb2f7bf6f
📒 Files selected for processing (9)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AnnouncementsCacheStoreImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/DeviceIdentityRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/secure/DataStoreToKSafeMigrator.ktgradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- gradle/libs.versions.toml
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AnnouncementsCacheStoreImpl.kt
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.kt
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.kt
…ors, gate slow-download on mirror migration
rainxchzed
force-pushed
the
feat/ksafe-migration
branch
from
c8d93b4 to
81d2f6c
Compare
Comment on lines
+130
to
+151
| ksafe.delete(keys.username); ksafe.delete(keys.password) | ||
| } | ||
| is ProxyConfig.Http -> { | ||
| ksafe.put(keys.type, "http") | ||
| ksafe.put(keys.host, config.host) | ||
| ksafe.put(keys.port, config.port) | ||
| writeOrClear(keys.username, config.username) | ||
| writeOrClear(keys.password, config.password) | ||
| } | ||
| is ProxyConfig.Socks -> { | ||
| ksafe.put(keys.type, "socks") | ||
| ksafe.put(keys.host, config.host) | ||
| ksafe.put(keys.port, config.port) | ||
| writeOrClear(keys.username, config.username) | ||
| writeOrClear(keys.password, config.password) | ||
| } | ||
| } | ||
| ProxyManager.setConfig(scope, config) | ||
| } | ||
|
|
||
| private suspend fun writeOrClear(key: String, value: String?) { | ||
| if (value != null) ksafe.put(key, value) else ksafe.delete(key) |
There was a problem hiding this comment.
edit {} atomicity
The old preferences.edit { prefs -> ... } block was a single atomic DataStore transaction. The new code issues up to 5 independent ksafe.put() / ksafe.delete() calls sequentially. If the process is killed between ksafe.put(keys.type, "http") and the subsequent ksafe.put(keys.host, config.host), the vault ends up with type = "http" but no host. On the next launch, parseConfig sees validHost = null and silently falls back to ProxyConfig.System, losing the user's configured proxy without any visible error. The same partial-write window exists for all compound config types (Http, Socks). KSafe does not appear to expose a transactional / batch-write API, so consider writing a single serialized object (e.g. a small @Serializable ProxyScopeConfig dataclass) per scope rather than five independent scalar keys.
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
First phase of DataStore → KSafe migration. KSafe (https://github.com/ioannisa/KSafe) gives us AES-256-GCM encryption by default, hardware-backed Keystore on Android, single-instance KMP API across Android + JVM.
This PR: most security-sensitive store first — GitHub OAuth tokens.
Architecture
gradle/libs.versions.toml—ksafe = "2.0.0"+ library entrycore/data/build.gradle.kts— addedimplementation(libs.ksafe)core/data/src/{android,jvm}Main/.../di/PlatformModule.{android,jvm}.kt— registerKSafe(fileName = "ghs-tokens")singleton under qualifier"tokens"DefaultTokenStore:DataStore<Preferences>+ manual JSON encode/decode ofGithubDeviceTokenSuccessDtoin plaintext string prefKSafe+legacyDataStorefor one-shot migration. Token stored as@Serializableobject directly (KSafe handles JSON + encryption transparently)legacyDataStoreinjected so migration shim has access; can drop the legacy dep after a couple of releasesOut of scope (follow-up PRs)
TweaksRepositoryImpl— biggest consumer (~35 prefs); Plain mode for cosmetic prefs is fineProxyRepositoryImpl— proxy credentials (encrypted)MirrorRepositoryImpl/MirrorPersistenceAnnouncementsCacheStoreImplDeviceIdentityRepositoryImplExternalImportRepositoryImplSlowDownloadDetectorImplEach consumer migrates in its own PR with the same auto-migration shim pattern this PR establishes.
Test plan
ghs-tokens.preferences_pb— token bytes are ciphertext, not JSONSummary by CodeRabbit