feat: production wiring for encryption disable/rotate + recovery UX (Phase 4, #338 items 1+2) - #342
Merged
Merged
Conversation
…ble/rotate (#338 items 1+2) Replaces the clearIdbPassphrase/rotateIdbPassphrase stubs with real journal-backed migrations: new primary-store adapters (images, binder assets, codex, app-data, snapshots, RAG vectors) plug into the existing secondary-store adapters via a new encryptionMigrationOrchestrator.ts, giving the previously test-only migration engine its first production callers. Disable/rotate now migrate and verify every protected store before touching the passphrase sentinel, and fail closed into the resumable journal (not a partial verifier change) on error. Breaks a circular import (storageEncryptionService -> orchestrator -> adapters -> storageEncryptionService) via a dynamic import at the two call sites, matching this codebase's existing dynamic-import convention for avoiding heavy/circular static coupling (listenerMiddleware.ts, aiApi.ts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…st encryption (#338 items 1+2) Extends PassphraseModal with 'disable'/'rotate' modes (live migration progress bar), adds Disable/Change-passphrase buttons to PrivacySection, and wires both through useSettingsView to the real clearIdbPassphrase/rotateIdbPassphrase implementations. Adds EncryptionRecoveryModal + an App.tsx startup check: if a disable/rotate journal was interrupted (reload/crash) before reaching 'completed', it takes priority over the normal unlock flow and lets the user re-enter their passphrase(s) to safely resume — CryptoKey material is never persisted, only re-derivable. A 'recovery-required' journal (the migration's own verification found an inconsistency) is surfaced as a distinct, honest stuck state rather than a fake auto-fix. Refactors clearIdbPassphrase/rotateIdbPassphrase's post-migration commit steps and key/verifier derivation into shared helpers so resumeEncryptionMigration() (used by the recovery UX) reaches the exact same end state as a fresh-start migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ry UX (#338) Real-data round-trip tests (image, codex 3-shape dispatch, binder asset Blob<->bytes, RAG vectors aggregate<->individual duality) through rekey then disable — the prior disable/rotate tests only proved migration completes over empty stores. Adds resumeEncryptionMigration coverage (correct resume, wrong source passphrase, missing target passphrase for rekey), new EncryptionRecoveryModal component tests, and disable/rotate mode + progress-bar tests for PassphraseModal, PrivacySection's new Change/Disable buttons, and useSettingsView's handlePassphraseConfirm branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Updates CLAUDE.md § At-rest encryption and Known Technical Debt, docs/IDB-ENCRYPTION.md, and ADR-0018's status line to reflect the production migration wiring implemented in this branch — the orchestrator, primary-store adapters, protectedWriteAdmission cross-tab gate, and EncryptionRecoveryModal. Remaining gap noted honestly: no E2E coverage yet for the disable/rotate/recovery round trips (unit + component tests only so far). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideWires the previously test-only IDB encryption migration journal into production disable/rotate flows by adding primary-store adapters, a migration orchestrator with progress callbacks, real clear/rotate implementations with recovery, and corresponding settings/recovery UI, while updating docs and tests to reflect the complete lifecycle. Sequence diagram for disable/rotate migration flow from SettingssequenceDiagram
actor User
participant PrivacySection
participant PassphraseModal
participant storageEncryptionService
participant encryptionMigrationOrchestrator
participant protectedStoreMigration
User ->> PrivacySection: Click Disable/Rotate buttons
PrivacySection ->> PassphraseModal: setPassphraseModal('disable'|'rotate')
User ->> PassphraseModal: Enter passphrase(s), confirm
PassphraseModal ->> PrivacySection: onConfirm(current, next)
PrivacySection ->> storageEncryptionService: clearIdbPassphrase(onProgress) / rotateIdbPassphrase(oldPassphrase, newPassphrase, onProgress)
storageEncryptionService ->> encryptionMigrationOrchestrator: runProductionEncryptionMigration({operation, keys, targetVerifier?, onProgress})
encryptionMigrationOrchestrator ->> protectedStoreMigration: runProtectedStoreMigration(journal, adapters, keys, onProgress)
protectedStoreMigration -->> PassphraseModal: onProgress(progress)
PassphraseModal -->> User: Migration progress bar
protectedStoreMigration -->> encryptionMigrationOrchestrator: journal (phase 'committing')
encryptionMigrationOrchestrator -->> storageEncryptionService: journal
storageEncryptionService ->> storageEncryptionService: commitDisableMigration(journal) / commitRekeyMigration(journal, targetKey)
storageEncryptionService -->> PrivacySection: success
PrivacySection -->> User: Toast (disabled/changed), updated encryptionReady
Sequence diagram for interrupted migration recovery on startupsequenceDiagram
actor User
participant App
participant EncryptionRecoveryModal
participant storageEncryptionService
participant encryptionMigrationOrchestrator
participant protectedStoreMigration
App ->> storageEncryptionService: readEncryptionMigrationJournal()
storageEncryptionService -->> App: journal (phase !== 'completed')
App ->> EncryptionRecoveryModal: journal prop
User ->> EncryptionRecoveryModal: Enter passphrase(s), Resume
EncryptionRecoveryModal ->> storageEncryptionService: resumeEncryptionMigration(journal, {sourcePassphrase, targetPassphrase?}, onProgress)
storageEncryptionService ->> storageEncryptionService: deriveAndVerifySourceKeyFromSentinel()
storageEncryptionService ->> storageEncryptionService: deriveAndVerifyTargetKeyFromVerifier() [rekey]
storageEncryptionService ->> encryptionMigrationOrchestrator: resumeProductionEncryptionMigration(journal, keys, onProgress)
encryptionMigrationOrchestrator ->> protectedStoreMigration: runProtectedStoreMigration(journal, adapters, keys, onProgress)
protectedStoreMigration -->> EncryptionRecoveryModal: onProgress(progress)
EncryptionRecoveryModal -->> User: Migration progress bar
protectedStoreMigration -->> encryptionMigrationOrchestrator: journal (phase 'committing')
encryptionMigrationOrchestrator -->> storageEncryptionService: journal
storageEncryptionService ->> storageEncryptionService: commitDisableMigration(journal) / commitRekeyMigration(journal, targetKey)
storageEncryptionService -->> EncryptionRecoveryModal: success
EncryptionRecoveryModal ->> App: onRecovered()
App ->> App: setEnableIdbAtRestEncryption(false) [disable]
App -->> User: Normal unlock flow resumes
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5 tasks
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIndexedDB encryption now supports production disable and passphrase rotation across registered protected stores. Migrations persist journals, report progress, verify converted data, and resume after interruption through startup recovery UI. ChangesEncryption migration engine
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score:🔵 Low · up to The PR adds production encryption disable/rotation and recovery flows. It is mergeable with explicit owner follow-up for two bounded security-maintenance issues: an executable test file is broadly excluded from secret scanning, and a vulnerability ignore record is malformed or mismatched to its documented advisory. Sequence Diagram(s)sequenceDiagram
participant User
participant PrivacySection
participant useSettingsView
participant storageEncryptionService
participant encryptionMigrationOrchestrator
participant ProtectedStores
User->>PrivacySection: select disable or rotate
PrivacySection->>useSettingsView: submit passphrase operation
useSettingsView->>storageEncryptionService: start migration
storageEncryptionService->>encryptionMigrationOrchestrator: create journal and migrate
encryptionMigrationOrchestrator->>ProtectedStores: transform and verify records
ProtectedStores-->>useSettingsView: migration progress
encryptionMigrationOrchestrator-->>storageEncryptionService: completed journal
storageEncryptionService-->>useSettingsView: commit encryption state
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Both primaryProtectedStoreAdapter.ts and ragVectorsProtectedStoreAdapter.ts implement very similar openExistingDatabase/withDatabase logic; consider extracting a shared IndexedDB helper to reduce duplication and keep adapter code focused on migration semantics.
- The progress fraction and progress bar rendering logic is duplicated between PassphraseModal and EncryptionRecoveryModal; a small shared utility for converting ProtectedStoreMigrationProgress to a normalized percentage would make the UX code easier to maintain and keep the behavior consistent.
- ProtectedStoreMigrationProgress currently reports per-store index and processed counts, but the UI only uses storeIndex/storeCount and ignores processed; it may be worth clarifying or documenting the intended semantics (stores vs. records) so future consumers don’t misinterpret or overfit to the current usage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- Both primaryProtectedStoreAdapter.ts and ragVectorsProtectedStoreAdapter.ts implement very similar openExistingDatabase/withDatabase logic; consider extracting a shared IndexedDB helper to reduce duplication and keep adapter code focused on migration semantics.
- The progress fraction and progress bar rendering logic is duplicated between PassphraseModal and EncryptionRecoveryModal; a small shared utility for converting ProtectedStoreMigrationProgress to a normalized percentage would make the UX code easier to maintain and keep the behavior consistent.
- ProtectedStoreMigrationProgress currently reports per-store index and processed counts, but the UI only uses storeIndex/storeCount and ignores processed; it may be worth clarifying or documenting the intended semantics (stores vs. records) so future consumers don’t misinterpret or overfit to the current usage.
## Individual Comments### Comment 1
<locationpath="tests/unit/hooks/useSettingsView.test.ts"line_range="605-614" />
<code_context>
+describe('handlePassphraseConfirm — disable/rotate', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add rotate-failure coverage to mirror the disable-failure behavior
Current tests cover disable success, rotate success, and disable failure, but not rotate failure (e.g. `mockRotateIdbPassphrase` rejecting). Since the expected behavior on rotate error is to keep the modal open, clear `migrationProgress`, and avoid a success toast, please add a test that mocks `rotateIdbPassphrase` rejecting and asserts:
-`migrationProgress` is cleared
-`passphraseModal` stays `'rotate'`-`mockToastSuccess` is not called.
This will bring rotate failure coverage in line with disable and verify the hook’s error handling more thoroughly.
Suggested implementation:
```typescriptdescribe('handlePassphraseConfirm — disable/rotate', () => {
afterEach(() => {
mockClearIdbPassphrase.mockResolvedValue(undefined);
mockRotateIdbPassphrase.mockResolvedValue(undefined);
});
it('clears migrationProgress, keeps passphraseModal as rotate, and does not toast on rotate failure', async () => {
const { result } =renderHook(() =>useSettingsView());
// Simulate a rotate errormockRotateIdbPassphrase.mockRejectedValue(newError('rotate failed'));
act(() => {
result.current.setPassphraseModal('rotate');
});
awaitact(async () => {
awaitresult.current.handlePassphraseConfirm();
});
// migrationProgress is cleared on errorexpect(result.current.migrationProgress).toBeNull();
// modal remains open in rotate stateexpect(result.current.passphraseModal).toBe('rotate');
// no success toast on errorexpect(mockToastSuccess).not.toHaveBeenCalled();
});
```To align this with the rest of the test file, you may need to:1. Adjust the `migrationProgress` assertion (`toBeNull()` vs `toBeUndefined()` or another expected cleared value) to match how the hook actually resets `migrationProgress` on error (mirror whatever the existing disable-failure test asserts).2. Ensure `mockRotateIdbPassphrase` and `mockToastSuccess` are already defined/mocked at the top of the file; if not, add appropriate `vi.fn()` mocks consistent with the existing conventions.3. If other tests use `waitFor` instead of `awaitact(async () =>...)` for async assertions on hook state, you can switch to `waitFor` to match the existing style.</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Uh oh!
There was an error while loading. Please reload this page.
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36028823 | Triggered | Generic Password | 0ddcb44 | tests/unit/storage/storageEncryptionService.test.ts | View secret |
| 36028823 | Triggered | Generic Password | 423c115 | tests/unit/storage/storageEncryptionService.test.ts | View secret |
| 36042393 | Triggered | Generic Password | 423c115 | tests/unit/storage/storageEncryptionService.test.ts | View secret |
| 36028823 | Triggered | Generic Password | 423c115 | tests/unit/storage/storageEncryptionService.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
App.tsx (1)
354-379: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the journal read a startup barrier.
recoveryJournalstarts asnull, so the unlock effect can openIdbUnlockModalbeforereadEncryptionMigrationJournal()resolves. This breaks the recovery-first contract on slow IndexedDB reads.The async journal read also has no
catch. A rejected read creates an unhandled rejection and leaves no safe recovery state.Track
checking,none, andjournalstates. Do not run normal unlock until the check completes. If the read fails, keep recovery blocked, show an actionable recovery error, and log sanitized diagnostics throughservices/logger.ts.As per coding guidelines, async operations must use
try/catch, and technical details must useservices/logger.ts.🤖 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 `@App.tsx` around lines 354 - 379, Make the migration-journal read in the startup useEffect an explicit checking/none/journal state instead of using null as the initial value, and gate the normal unlock effect until the check reaches none. Wrap readEncryptionMigrationJournal in try/catch; on failure keep unlock blocked, expose an actionable recovery error state, and log only sanitized diagnostics through services/logger.ts. Update the recoveryJournal checks and effect dependencies to preserve recovery-first behavior.Source: Coding guidelines
🧹 Nitpick comments (12)
components/settings/PassphraseModal.tsx (1)
78-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the
QNBS-v3rationale on one physical line.Lines 78-80 wrap the added
QNBS-v3comment. Replace it with one physical line.As per coding guidelines, “For every non-trivial code change, add one single-line
QNBS-v3comment explaining why … never wrap the comment across physical lines.”🤖 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 `@components/settings/PassphraseModal.tsx` around lines 78 - 80, Update the QNBS-v3 rationale comment in PassphraseModal so the entire explanation is on one physical line, preserving its current meaning and wording without changing surrounding logic.Source: Coding guidelines
components/settings/PrivacySection.tsx (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the
QNBS-v3rationale on one physical line.Lines 116-117 split the added
QNBS-v3comment across two lines. Condense this rationale into one physical comment line.As per coding guidelines, “For every non-trivial code change, add one single-line
QNBS-v3comment explaining why … never wrap the comment across physical lines.”🤖 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 `@components/settings/PrivacySection.tsx` around lines 116 - 117, Condense the QNBS-v3 rationale comment near clearIdbPassphrase/rotateIdbPassphrase onto one physical line, preserving its explanation that both require an unlocked session key and re-migrate protected stores before touching the sentinel.Source: Coding guidelines
tests/unit/settings/PrivacySection.test.tsx (1)
163-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a one-line
QNBS-v3rationale for the new action-flow tests.These tests add rotate and disable interaction coverage. Add one physical-line
QNBS-v3comment that states why this regression coverage is required.As per coding guidelines, “For every non-trivial code change, add one single-line
QNBS-v3comment explaining why … never wrap the comment across physical lines.”🤖 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 `@tests/unit/settings/PrivacySection.test.tsx` around lines 163 - 188, Add one physical-line QNBS-v3 comment near the rotate and disable action-flow tests explaining that the regression coverage verifies the correct passphrase modal opens for each encryption action. Keep the comment single-line and avoid adding additional comments or wrapping it across lines.Source: Coding guidelines
tests/unit/settings/PassphraseModal.test.tsx (1)
228-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a one-line
QNBS-v3rationale for the new modal-mode tests.These suites add disable and rotate migration coverage. Add one physical-line
QNBS-v3comment that states why this coverage exists.As per coding guidelines, “For every non-trivial code change, add one single-line
QNBS-v3comment explaining why … never wrap the comment across physical lines.”🤖 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 `@tests/unit/settings/PassphraseModal.test.tsx` around lines 228 - 413, Add one single-line QNBS-v3 comment adjacent to the new PassphraseModal disable and rotate test suites, stating that the coverage verifies migration behavior and mode-specific passphrase flows. Keep the rationale on exactly one physical line and do not add additional comments.Source: Coding guidelines
services/storage/encryptionMigrationOrchestrator.ts (1)
27-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert unique adapter ids when combining registries.
The journal maps checkpoints by
adapter.id. If a primary adapter and a secondary adapter ever share an id, two stores share one checkpoint. The migration then reports progress and verification for the wrong store and can mark an unmigrated store as done. Fail fast on a duplicate id instead.♻️ Suggested guard
function getRegisteredProtectedStoreAdapters(): readonly ProtectedStoreAdapter[] { - return [+ const adapters = [ ...getRegisteredPrimaryProtectedStoreAdapters(), ...getRegisteredSecondaryProtectedStoreAdapters(), ]; + // QNBS-v3: journal checkpoints are keyed by adapter id — a collision would silently share one checkpoint between two stores.+ const ids = new Set(adapters.map((adapter) => adapter.id));+ if (ids.size !== adapters.length) {+ throw new Error('Duplicate protected-store adapter id in the migration registry');+ }+ return adapters; }🤖 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 `@services/storage/encryptionMigrationOrchestrator.ts` around lines 27 - 32, Update getRegisteredProtectedStoreAdapters to validate that every combined ProtectedStoreAdapter has a unique id across the primary and secondary registries, throwing immediately when a duplicate is detected before returning the array.services/storage/ragVectorsProtectedStoreAdapter.ts (1)
57-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
openExistingDatabaseis duplicated.This function is a verbatim copy of
openExistingDatabaseinservices/storage/primaryProtectedStoreAdapter.ts(Lines 62-97). Its abort-on-create semantics are subtle, so two copies will diverge. Extract it into one shared storage helper and import it in both adapters.As per coding guidelines: "Apply DRY: place reusable logic in services, hooks, or feature thunks instead of duplicating it in views."
🤖 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 `@services/storage/ragVectorsProtectedStoreAdapter.ts` around lines 57 - 104, The openExistingDatabase logic is duplicated between ragVectorsProtectedStoreAdapter and primaryProtectedStoreAdapter. Extract the shared openExistingDatabase implementation into a reusable storage helper, then import and use that helper from both adapters while preserving its abort-on-create and existing error-handling behavior.Source: Coding guidelines
services/storage/storageEncryptionService.ts (2)
736-737: 🔒 Security & Privacy | 🔵 TrivialPassphrase rotation keeps the existing KDF salt.
getExistingSalt()derives the new key with the old salt. This is required by the resume design, because the journal stores only a target verifier and the key must stay re-derivable. It also means a rotation does not change the KDF input material. Record this trade-off indocs/IDB-ENCRYPTION.mdor the ADR, so a future change to re-salt on rotation also plans journal-side salt versioning.🤖 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 `@services/storage/storageEncryptionService.ts` around lines 736 - 737, Document in docs/IDB-ENCRYPTION.md or the relevant ADR that passphrase rotation in the flow using _svc.deriveKey and getExistingSalt intentionally reuses the existing KDF salt so the target key remains re-derivable during resume from the journal. Record that future re-salting must include journal-side salt versioning.
662-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThis file now exceeds the 700-line target.
The new commit, derive, and resume helpers push
storageEncryptionService.tspast 817 lines. Consider moving the migration lifecycle helpers,commitDisableMigration,commitRekeyMigration,deriveAndVerifySourceKeyFromSentinel,deriveAndVerifyTargetKeyFromVerifier, andresumeEncryptionMigration, into a dedicated module. The crypto primitives and the session-key state then stay separate from the lifecycle orchestration.As per coding guidelines: "Target files between 200 and 700 lines; split files over 700 lines into hooks, subcomponents, selectors, or tests rather than using comment-only sections."
🤖 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 `@services/storage/storageEncryptionService.ts` around lines 662 - 663, Extract the migration lifecycle orchestration from storageEncryptionService.ts into a dedicated module, including the migration lifecycle helpers, commitDisableMigration, commitRekeyMigration, deriveAndVerifySourceKeyFromSentinel, deriveAndVerifyTargetKeyFromVerifier, and resumeEncryptionMigration. Update imports and call sites so crypto primitives and session-key state remain in storageEncryptionService.ts while all existing migration behavior is preserved.Source: Coding guidelines
services/storage/protectedStoreMigration.ts (1)
271-277: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate progress-callback failures from the migration run.
onProgressis supplied by UI code. If a callback throws, the exception propagates out of the migrating loop, aborts the run, and releases the lease, even though every store write and journal checkpoint succeeded. Wrap the invocation so reporting cannot fail a crypto migration.♻️ Suggested guard (apply to both call sites)
+// QNBS-v3: progress reporting is observability only — a throwing UI callback must never abort a durable migration.+function reportProgress(+ onProgress: ProtectedStoreMigrationProgressCallback | undefined,+ progress: ProtectedStoreMigrationProgress,+): void {+ if (!onProgress) return;+ try {+ onProgress(progress);+ } catch {+ /* ignore reporting failures */+ }+}- onProgress?.({- storeId: adapter.id,- storeIndex,- storeCount: adapters.length,- phase: 'migrating',- processed: checkpoint.processed,- });+ reportProgress(onProgress, {+ storeId: adapter.id,+ storeIndex,+ storeCount: adapters.length,+ phase: 'migrating',+ processed: checkpoint.processed,+ });🤖 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 `@services/storage/protectedStoreMigration.ts` around lines 271 - 277, Wrap both onProgress invocations in the migration flow, including the call near the migrating phase, with error isolation so exceptions from UI callbacks are caught and do not propagate into the store loop or abort the migration. Keep progress reporting best-effort while preserving the existing migration, checkpoint, and lease behavior.services/storage/primaryProtectedStoreAdapters.ts (1)
77-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe rekey fallback discards the real failure cause in every adapter. Each adapter probes the target key first, then falls back to the source key inside a nested
try. The innercatchrethrows the target-probe error and drops the source-side error, so a failed rotation always reports the wrong cause.
services/storage/primaryProtectedStoreAdapters.ts#L77-L88: keep the source-decrypt error ascauseon the thrownProtectedStoreMigrationAdapterError; apply the same change at Lines 270-285 and Lines 400-410.services/storage/ragVectorsProtectedStoreAdapter.ts#L181-L207: moverequireKey(context.sourceKey, 'source')out of the innertryand attach the source-decrypt error ascause.🤖 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 `@services/storage/primaryProtectedStoreAdapters.ts` around lines 77 - 88, Update the rekey fallback error handling in services/storage/primaryProtectedStoreAdapters.ts at lines 77-88, 270-285, and 400-410 so the source-key decryption failure is thrown as ProtectedStoreMigrationAdapterError with that error attached as cause, rather than rethrowing the target-probe error. In services/storage/ragVectorsProtectedStoreAdapter.ts at lines 181-207, move requireKey(context.sourceKey, 'source') outside the inner try and attach the source-decrypt error as cause; apply this consistently across all affected adapters.tests/unit/storage/storageEncryptionService.test.ts (1)
746-779: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the nondeterministic operation ID and the duplicated salt key.
Line 768 builds
operationIdfromMath.random(). Line 748 hardcodes the KDF salt storage key'worldscript-idb-kdf-salt-v1'. A counter keeps the ID deterministic, and importing the production constant keeps the helper aligned if the key changes.As per coding guidelines: "Make tests deterministic by mocking
Date.now(), using fake timers, resetting global state inbeforeEach, and never relying on network or test order."♻️ Proposed deterministic operation ID
+ // QNBS-v3: deterministic operationId — a counter keeps journal IDs reproducible across runs.+ let recoveryJournalSeq = 0;+ async function buildPendingRekeyJournal(targetPassphrase: string) { @@ return beginEncryptionMigration({ - operationId: `recovery-test-${Math.random().toString(36).slice(2)}`,+ operationId: `recovery-test-${++recoveryJournalSeq}`,🤖 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 `@tests/unit/storage/storageEncryptionService.test.ts` around lines 746 - 779, Update readCurrentSaltForTest to use the production KDF salt storage-key constant instead of the hardcoded string, and replace the Math.random()-based operationId in buildPendingRekeyJournal with a deterministic counter that is reset between tests.Source: Coding guidelines
tests/unit/storageEncryptionService.test.ts (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the reserved record key from the real module.
Line 26 duplicates the production value of
PASSPHRASE_SENTINEL_RECORD_KEYas a string literal. If the production constant changes, this mock keeps the old value andprimaryProtectedStoreAdapters.tsreceives a stale reserved key while the suite stays green. Re-export the real constant inside the mock factory instead.♻️ Proposed fix using the actual module value
- // QNBS-v3: primaryProtectedStoreAdapters.ts reads this reserved-key constant at module load —- // the mock factory must supply it too, not just the three sentinel CRUD functions.- PASSPHRASE_SENTINEL_RECORD_KEY: 'idb_passphrase_sentinel_v1',+ // QNBS-v3: primaryProtectedStoreAdapters.ts reads this reserved-key constant at module load —+ // the mock factory must supply it too, and it must track the real value to avoid silent drift.+ PASSPHRASE_SENTINEL_RECORD_KEY: (+ await vi.importActual<typeof import('../../services/storage/idbPassphraseSentinel')>(+ '../../services/storage/idbPassphraseSentinel',+ )+ ).PASSPHRASE_SENTINEL_RECORD_KEY,Adjust the module path to the real sentinel module path, and make the mock factory
async.🤖 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 `@tests/unit/storageEncryptionService.test.ts` around lines 24 - 27, Update the mock factory in storage encryption tests to derive PASSPHRASE_SENTINEL_RECORD_KEY from the real sentinel module instead of duplicating the string literal. Use the correct production module path and make the mock factory async so it can import and re-export the actual constant while preserving the existing mocked CRUD functions.
🤖 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 `@App.tsx`:
- Around line 206-207: Convert the QNBS-v3 annotation at App.tsx lines 206-207
into one physical line; make the same change at App.tsx lines 354-356. Add one
physical-line QNBS-v3 comment for the recovery behavior in
components/settings/EncryptionRecoveryModal.tsx lines 17-24, and replace the
wrapped annotation in tests/unit/settings/EncryptionRecoveryModal.test.tsx lines
1-5 with one physical line. Each non-trivial change must retain a single-line
QNBS-v3 comment without wrapping.
- Around line 829-840: Update the recovery UI around EncryptionRecoveryModal in
App so an ErrorBoundary failure still renders a recovery-specific blocking
fallback with a backdrop and focus trap, or move the recovery guard outside the
normal application tree. Preserve the recovery lock for as long as
recoveryJournal is set and keep the existing onRecovered cleanup behavior
unchanged.
In `@components/settings/EncryptionRecoveryModal.tsx`:
- Line 120: Update both password input className declarations in
components/settings/EncryptionRecoveryModal.tsx at lines 120-120 and 144-144,
replacing the --sc-border-focus ring token with --sc-ring-focus while preserving
the required focus-visible:ring-2 classes.
- Around line 52-70: Update handleResume in EncryptionRecoveryModal to
distinguish credential-related failures from invalid journal, missing verifier,
and migration errors; retain encryptionWrongPassphrase only for credential
failures and show a separate actionable recovery-failure message otherwise. Log
sanitized technical context for non-credential failures through
services/logger.ts, and add a test covering a non-credential rejection.
In `@components/settings/PassphraseModal.tsx`:
- Line 14: Update the contract comment for onConfirm in PassphraseModal to state
that only unlock receives (passphrase, ''), while disable receives ('', '') and
the existing set and rotate contracts remain accurate.
- Around line 77-87: Update the catch handling in PassphraseModal’s
rotateIdbPassphrase flow so only typed old-passphrase verification failures use
encryptionWrongPassphrase; map journal, recovery-required, and migration
failures to brief actionable migration/recovery messages, and log sanitized
diagnostics through services/logger.ts. In
tests/unit/settings/PassphraseModal.test.tsx lines 392-412, retain the typed
old-passphrase assertion and add coverage for migration and recovery failures
with the new user-facing errors.
- Around line 196-217: Add aria-live="polite" to the migration progress status
text or its containing element in PassphraseModal’s busy progress block,
ensuring dynamic updates to the translated current/total progress are announced
without changing the existing progressbar behavior.
- Around line 129-130: Update the input className declarations in
PassphraseModal so their focus-visible ring utility uses the required
--sc-ring-focus token instead of --sc-border-focus, preserving the existing
focus-visible:ring-2 styling across all referenced inputs.
- Around line 104-105: Update the Modal invocation in PassphraseModal to pass
isDismissible={!busy}, keeping backdrop and Escape dismissal disabled while
clearIdbPassphrase or rotateIdbPassphrase is active and restoring dismissal when
the migration completes.
In `@docs/adr/0018-idb-encryption-lifecycle-and-recovery.md`:
- Line 3: Update the contradictory lifecycle statements in the ADR sections
around the pending conversion policy, remaining work, and UI availability to
reflect that disable and rekey operations are shipped. Preserve the documented
RECOVERY_REQUIRED manual-recovery limitation, and identify remaining work such
as E2E coverage and manual recovery instead of obsolete lease or
lifecycle-enablement prerequisites.
In `@docs/IDB-ENCRYPTION.md`:
- Around line 145-158: The documentation incorrectly states that protected store
writers call assertIdbProtectedWriteAllowed() through
withProtectedWriteAdmission(). Update the protected-store writer call-path
description to reflect withProtectedWriteAdmission() →
resolveProtectedWriteKey() → assertNoActiveEncryptionMigration() before
transactions, while noting that delete paths invoke
assertIdbProtectedWriteAllowed() directly and commitDisableMigration removes the
KDF salt via clearStoredSalt() after deleting the sentinel.
In `@services/storage/primaryProtectedStoreAdapter.ts`:
- Around line 294-313: Make clearGeminiApiKey() and clearApiKey() acquire
withProtectedWriteAdmission before deleting APP_DATA_STORE records, matching
migration write coordination. Update verify() and its migration accounting to
tolerate records deleted after processing by using stable migration-scope or
deletion-aware counts, so valid deletions do not produce verified <
checkpoint.processed or recovery-required.
In `@services/storage/ragVectorsProtectedStoreAdapter.ts`:
- Around line 127-134: Update readProjectRecords to handle transaction.onabort
by rejecting the promise with the transaction’s error, while preserving the
existing request success and error handlers so the promise always settles during
transaction aborts.
In `@services/storage/storageEncryptionService.ts`:
- Around line 664-671: Update resumeEncryptionMigration and the commit helpers
in services/storage/storageEncryptionService.ts at lines 664-671 and 673-683:
for a disable journal in committing state with a missing sentinel, treat
migration as already applied and complete journal bookkeeping without deriving a
key; for a rekey journal in committing state, validate the entered passphrase
against the current stored sentinel, including when it is already the new
sentinel, before reporting an incorrect passphrase.
In `@tests/unit/settings/EncryptionRecoveryModal.test.tsx`:
- Around line 34-50: Update the Modal mock in EncryptionRecoveryModal tests to
accept and expose isDismissible, then assert it is false on the rendered
recovery dialog. Preserve the existing isOpen behavior while ensuring tests fail
if the recovery modal becomes dismissible.
In `@tests/unit/storage/storageEncryptionService.test.ts`:
- Around line 627-655: Update the migration test setup, especially the shared
beforeEach used by clearIdbPassphrase and rotateIdbPassphrase tests, to reset
all relevant IndexedDB databases and cached connections in addition to the
sentinel and local storage. Ensure images, codex records, binder assets, and
vectors are removed between tests so later migrations start with isolated state.
---
Outside diff comments:
In `@App.tsx`:
- Around line 354-379: Make the migration-journal read in the startup useEffect
an explicit checking/none/journal state instead of using null as the initial
value, and gate the normal unlock effect until the check reaches none. Wrap
readEncryptionMigrationJournal in try/catch; on failure keep unlock blocked,
expose an actionable recovery error state, and log only sanitized diagnostics
through services/logger.ts. Update the recoveryJournal checks and effect
dependencies to preserve recovery-first behavior.
---
Nitpick comments:
In `@components/settings/PassphraseModal.tsx`:
- Around line 78-80: Update the QNBS-v3 rationale comment in PassphraseModal so
the entire explanation is on one physical line, preserving its current meaning
and wording without changing surrounding logic.
In `@components/settings/PrivacySection.tsx`:
- Around line 116-117: Condense the QNBS-v3 rationale comment near
clearIdbPassphrase/rotateIdbPassphrase onto one physical line, preserving its
explanation that both require an unlocked session key and re-migrate protected
stores before touching the sentinel.
In `@services/storage/encryptionMigrationOrchestrator.ts`:
- Around line 27-32: Update getRegisteredProtectedStoreAdapters to validate that
every combined ProtectedStoreAdapter has a unique id across the primary and
secondary registries, throwing immediately when a duplicate is detected before
returning the array.
In `@services/storage/primaryProtectedStoreAdapters.ts`:
- Around line 77-88: Update the rekey fallback error handling in
services/storage/primaryProtectedStoreAdapters.ts at lines 77-88, 270-285, and
400-410 so the source-key decryption failure is thrown as
ProtectedStoreMigrationAdapterError with that error attached as cause, rather
than rethrowing the target-probe error. In
services/storage/ragVectorsProtectedStoreAdapter.ts at lines 181-207, move
requireKey(context.sourceKey, 'source') outside the inner try and attach the
source-decrypt error as cause; apply this consistently across all affected
adapters.
In `@services/storage/protectedStoreMigration.ts`:
- Around line 271-277: Wrap both onProgress invocations in the migration flow,
including the call near the migrating phase, with error isolation so exceptions
from UI callbacks are caught and do not propagate into the store loop or abort
the migration. Keep progress reporting best-effort while preserving the existing
migration, checkpoint, and lease behavior.
In `@services/storage/ragVectorsProtectedStoreAdapter.ts`:
- Around line 57-104: The openExistingDatabase logic is duplicated between
ragVectorsProtectedStoreAdapter and primaryProtectedStoreAdapter. Extract the
shared openExistingDatabase implementation into a reusable storage helper, then
import and use that helper from both adapters while preserving its
abort-on-create and existing error-handling behavior.
In `@services/storage/storageEncryptionService.ts`:
- Around line 736-737: Document in docs/IDB-ENCRYPTION.md or the relevant ADR
that passphrase rotation in the flow using _svc.deriveKey and getExistingSalt
intentionally reuses the existing KDF salt so the target key remains
re-derivable during resume from the journal. Record that future re-salting must
include journal-side salt versioning.
- Around line 662-663: Extract the migration lifecycle orchestration from
storageEncryptionService.ts into a dedicated module, including the migration
lifecycle helpers, commitDisableMigration, commitRekeyMigration,
deriveAndVerifySourceKeyFromSentinel, deriveAndVerifyTargetKeyFromVerifier, and
resumeEncryptionMigration. Update imports and call sites so crypto primitives
and session-key state remain in storageEncryptionService.ts while all existing
migration behavior is preserved.
In `@tests/unit/settings/PassphraseModal.test.tsx`:
- Around line 228-413: Add one single-line QNBS-v3 comment adjacent to the new
PassphraseModal disable and rotate test suites, stating that the coverage
verifies migration behavior and mode-specific passphrase flows. Keep the
rationale on exactly one physical line and do not add additional comments.
In `@tests/unit/settings/PrivacySection.test.tsx`:
- Around line 163-188: Add one physical-line QNBS-v3 comment near the rotate and
disable action-flow tests explaining that the regression coverage verifies the
correct passphrase modal opens for each encryption action. Keep the comment
single-line and avoid adding additional comments or wrapping it across lines.
In `@tests/unit/storage/storageEncryptionService.test.ts`:
- Around line 746-779: Update readCurrentSaltForTest to use the production KDF
salt storage-key constant instead of the hardcoded string, and replace the
Math.random()-based operationId in buildPendingRekeyJournal with a deterministic
counter that is reset between tests.
In `@tests/unit/storageEncryptionService.test.ts`:
- Around line 24-27: Update the mock factory in storage encryption tests to
derive PASSPHRASE_SENTINEL_RECORD_KEY from the real sentinel module instead of
duplicating the string literal. Use the correct production module path and make
the mock factory async so it can import and re-export the actual constant while
preserving the existing mocked CRUD functions.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: db5da394-ac82-4b4c-a6e1-6a55cd8a194e
📒 Files selected for processing (22)
App.tsxCLAUDE.mdcomponents/settings/EncryptionRecoveryModal.tsxcomponents/settings/PassphraseModal.tsxcomponents/settings/PrivacySection.tsxdocs/IDB-ENCRYPTION.mddocs/adr/0018-idb-encryption-lifecycle-and-recovery.mdhooks/useSettingsView.tsservices/storage/encryptionMigrationJournal.tsservices/storage/encryptionMigrationOrchestrator.tsservices/storage/idbPassphraseSentinel.tsservices/storage/primaryProtectedStoreAdapter.tsservices/storage/primaryProtectedStoreAdapters.tsservices/storage/protectedStoreMigration.tsservices/storage/ragVectorsProtectedStoreAdapter.tsservices/storage/storageEncryptionService.tstests/unit/hooks/useSettingsView.test.tstests/unit/settings/EncryptionRecoveryModal.test.tsxtests/unit/settings/PassphraseModal.test.tsxtests/unit/settings/PrivacySection.test.tsxtests/unit/storage/storageEncryptionService.test.tstests/unit/storageEncryptionService.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Code Review by Qodo
1. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
PR Summary by QodoWire encryption migration journal into production disable/rotate + recovery UX
AI Description
Diagram
High-Level Assessment
Files changed (22) |
…Y false positive gitleaks' generic-api-key rule flags PASSPHRASE_SENTINEL_RECORD_KEY = 'idb_passphrase_sentinel_v1' (services/storage/idbPassphraseSentinel.ts, mirrored as a mock constant in storageEncryptionService.test.ts) purely because the variable name contains "PASSPHRASE" + "KEY" next to a sufficiently long, structured string. It's a static IndexedDB record-key identifier, not a credential — the app must know it deterministically to find the sentinel record via store.get/put/delete. Verified locally against the exact PR commit range with gitleaks 8.24.3 (matching CI): confirmed the finding reproduces without this config, and is suppressed with it, while an unrelated real-looking secret (Stripe key pattern) is still caught — the allowlist does not weaken detection elsewhere. No .gitleaks.toml existed before this; [extend] useDefault = true keeps every default rule active. Scoped to the exact leaked value only (not combined with a path filter) since gitleaks' top-level [allowlist] ORs paths/regexes rather than ANDing them — combining both would broaden the exemption to "anything in that file" instead of narrowing it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independently flagged by CodeAnt, CodeRabbit, and qodo-code-review on PR #342: 1. Disable/rekey recovery can permanently strand an interrupted migration. commitDisableMigration deletes the sentinel/salt BEFORE completing the journal; commitRekeyMigration saves the new sentinel BEFORE completing the journal. If completeEncryptionMigration() fails after either mutation, the journal stays 'committing' but resumeEncryptionMigration's source-key derivation now fails — the sentinel it needs is already gone (disable) or already replaced (rekey) — permanently blocking recovery. resumeEncryptionMigration now special-cases a journal already at 'committing': for disable, an absent sentinel means the commit already ran — finish the journal bookkeeping without deriving a key. For rekey, if the old passphrase no longer decrypts the sentinel, verify the new passphrase against the journal's durable target verifier (independent of the sentinel) before concluding the commit already happened, instead of reporting a false wrong-passphrase error. 2. Blob optimistic-write check only compared size + MIME type, not bytes. Two Blobs can share both while holding different content (e.g. a concurrent binder-asset re-upload), so the migration's freshness check would silently accept a stale value and overwrite a newer asset. valuesMatch is now async and compares actual bytes for Blobs. writeBatch is split into a read phase (readCurrentValues) and a write phase (putValues) so the async Blob comparison never happens inside a live IDB transaction's request handler — safe because the whole batch already runs under an exclusive withMigrationAdmission() lock, so no writer can interleave between the two. 3. The app-data primary-store adapter corrupted IdbKeyStore records. APP_DATA_STORE holds project/settings data AND IdbKeyStore's raw non-extractable CryptoKey (local_crypto_key_v2) plus per-provider encrypted API keys (api_key_<provider>_enc/_iv, legacy gemini_api_key_*). The adapter only reserved the migration journal + passphrase sentinel keys, so enabling/rotating/disabling encryption would JSON.stringify the CryptoKey (destroying it) and re-encrypt the already-encrypted API-key bytes as if they were plaintext project data — permanently losing every stored API key. idbKeyStore.ts now exports isKeyStoreRecordKey(), wired into the app-data adapter via a new isReservedKey predicate (reservedKeys alone can't express the dynamic per-provider key family). New regression coverage: 6 resumeEncryptionMigration committing-phase tests (both operations × not-yet-committed / partially-committed sub-states), a real disable+rekey round-trip proving a stored API key survives migration, and 11 direct unit tests for valuesMatch's Blob comparison + isKeyStoreRecordKey. All 98 pre-existing storageEncryptionService tests still pass unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ErrorBoundary: new `fallback` render-prop lets a boundary override the default non-blocking ErrorFallback; wired into App.tsx's EncryptionRecoveryModal boundary so a crash inside the recovery modal still renders a blocking, focus-trapped Modal instead of leaving the app underneath interactive during a pending disable/rekey recovery. - Condense every wrapped QNBS-v3 comment introduced this PR (App.tsx, EncryptionRecoveryModal.tsx) to the required single physical line. - docs/adr/0018: fix stale "disable/rekey rejected outright" and "cross-tab admission not shipped" claims to match the current implementation. - docs/IDB-ENCRYPTION.md: correct the protected-write call-path description — save paths snapshot the key via resolveProtectedWriteKey() then re-check only assertNoActiveEncryptionMigration() pre-transaction; delete paths call assertIdbProtectedWriteAllowed() directly. - tests/unit/storage/storageEncryptionService.test.ts: give every test a fresh IDBFactory instead of one shared factory for the whole file, via new _resetDbForTest()/_resetSentinelStoreForTest() exports alongside the existing __resetEncryptionMigrationJournalConnectionsForTest() — closes a latent cross-test leak where an encrypted record from one test could outlive its salt/sentinel into a later test. - tests/unit/hooks/useSettingsView.test.ts: add the rotate-failure coverage mirroring the existing disable-failure test. - tests/unit/ErrorBoundary.test.tsx: cover the new fallback prop. Assessed as already resolved by earlier commits in this PR (no further change needed, verified against current code): - PassphraseModal.tsx's rotate-mode catch already distinguishes a credential-mismatch IdbWrongPassphraseError (only ever thrown by the pre-migration old-passphrase check) from any other migration/journal failure, with test coverage for both paths. - primaryProtectedStoreAdapter.ts's verify()/migrateNext() both skip isReservedKey() records via readBatch(), so clearGeminiApiKey()/ clearApiKey() can no longer desync checkpoint.processed vs. verified — isKeyStoreRecordKey() already excludes them from migration scope entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- src-tauri/osv-scanner.toml: add the GHSA-jmr9-qjv8-65gv (extract-zip) ignore entry already present on the sibling fix/333-download-progress-metrics branch — no fixed version exists, transitive Playwright devDependency only. - .gitguardian.yaml: extend the existing "Generic Password" false-positive path exclusion to storageEncryptionService.test.ts, which pairs fake sourcePassphrase/targetPassphrase fixture literals ('original', 'new-pass', 'wrong-old', ...) exercising the disable/rotate/recovery flows against fake-indexeddb — same detector, same false-positive class already documented for the locale trees above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qnbs
commented
Aug 12, 2026
OwnerAuthor
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
tests/unit/ErrorBoundary.test.tsx (1)
139-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
user-eventfor button interactions.Lines 92 and 153 call
fireEvent.clickon buttons. UseuserEvent.setup()andawait user.click(...)in both tests.@testing-library/user-eventis already available.🤖 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 `@tests/unit/ErrorBoundary.test.tsx` around lines 139 - 155, Replace fireEvent.click with asynchronous user-event interactions in both affected button tests, including the test around the custom fallback reset callback. Initialize userEvent.setup(), make the tests async, and await user.click(...) while preserving the existing assertions.Source: Path instructions
🤖 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 `@locales/el/settings.json`:
- Line 674: Update the Greek translation value for
settings.privacy.encryptionDisableAction from the current wording to
Απενεργοποίηση, preserving the existing key and JSON structure.
In `@locales/fr/settings.json`:
- Line 697: Update locales/fr/settings.json at lines 697-697 so the final
sentence names the migration as unable to resume. Apply equivalent wording in
locales/it/settings.json, then regenerate public/locales/it/bundle.json at lines
2343-2343; update both affected messages without changing other translation
content.
In `@locales/ko/settings.json`:
- Line 674: Correct the settings.privacy.encryptionDisableAction translation in
locales/ko/settings.json lines 674-674 to 비활성화 and locales/ru/settings.json
lines 674-674 to Отключить; update locales/el/settings.json to Απενεργοποίηση
and regenerate public/locales/el/bundle.json lines 2320-2320 from that source.
- Around line 678-679: Update the Korean at-rest encryption translations
associated with settings.privacy.encryptionDisabledStatus,
settings.privacy.encryptionEnabled, and the corresponding modal-title string to
consistently use 저장 시 암호화 instead of 미사용, preserving the existing meaning and
message structure.
In `@locales/pt/settings.json`:
- Line 239: Update the translation value for settings.ai.providerStatusReady
from the infinitive “Preparar” to the Portuguese ready-status term “Pronto”,
preserving the existing key.
In `@public/locales/fi/bundle.json`:
- Line 2326: Update the Finnish translations for encryptionForgotPassphrase,
encryptionPassphrase, encryptionChangeAction, encryptionChangeButton,
encryptionSetAction, and encryptionWarning to use “tunnuslause” consistently
instead of “salasana,” preserving the existing meaning and formatting.
In `@services/storage/storageEncryptionService.ts`:
- Around line 857-882: Restrict the existing try/catch around
deriveAndVerifySourceKeyFromSentinel so only source-passphrase verification can
enter the recovery branch. After successful verification, derive the target key
and call commitRekeyMigration outside that catch; only a source-verification
failure may validate the target verifier and invoke completeEncryptionMigration,
clearCompletedEncryptionMigration, and update the active-key state.
In `@tests/unit/storage/storageEncryptionService.test.ts`:
- Around line 1001-1025: Extend the failed rekey recovery test around
resumeEncryptionMigration to assert that the migration journal still exists and
hasPassphraseSentinel() remains true after rejection. Keep these checks in the
finally-safe flow before deleting the journal, so cleanup does not mask whether
failed recovery preserved both records.
---
Nitpick comments:
In `@tests/unit/ErrorBoundary.test.tsx`:
- Around line 139-155: Replace fireEvent.click with asynchronous user-event
interactions in both affected button tests, including the test around the custom
fallback reset callback. Initialize userEvent.setup(), make the tests async, and
await user.click(...) while preserving the existing assertions.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 704e1664-306d-476a-aab3-feab2d8ff821
📒 Files selected for processing (59)
.gitleaks.tomlApp.tsxcomponents/settings/EncryptionRecoveryModal.tsxcomponents/settings/PassphraseModal.tsxcomponents/settings/PrivacySection.tsxcomponents/ui/ErrorBoundary.tsxdocs/IDB-ENCRYPTION.mddocs/adr/0018-idb-encryption-lifecycle-and-recovery.mdlocales/ar/settings.jsonlocales/de/settings.jsonlocales/el/settings.jsonlocales/en/settings.jsonlocales/es/settings.jsonlocales/eu/settings.jsonlocales/fa/settings.jsonlocales/fi/settings.jsonlocales/fr/settings.jsonlocales/he/settings.jsonlocales/hu/settings.jsonlocales/is/settings.jsonlocales/it/settings.jsonlocales/ja/settings.jsonlocales/ko/settings.jsonlocales/pt/settings.jsonlocales/ru/settings.jsonlocales/sv/settings.jsonlocales/zh/settings.jsonpublic/locales/ar/bundle.jsonpublic/locales/de/bundle.jsonpublic/locales/el/bundle.jsonpublic/locales/en/bundle.jsonpublic/locales/es/bundle.jsonpublic/locales/eu/bundle.jsonpublic/locales/fa/bundle.jsonpublic/locales/fi/bundle.jsonpublic/locales/fr/bundle.jsonpublic/locales/he/bundle.jsonpublic/locales/hu/bundle.jsonpublic/locales/is/bundle.jsonpublic/locales/it/bundle.jsonpublic/locales/ja/bundle.jsonpublic/locales/ko/bundle.jsonpublic/locales/pt/bundle.jsonpublic/locales/ru/bundle.jsonpublic/locales/sv/bundle.jsonpublic/locales/zh/bundle.jsonservices/storage/idbKeyStore.tsservices/storage/idbPassphraseSentinel.tsservices/storage/index.tsservices/storage/primaryProtectedStoreAdapter.tsservices/storage/primaryProtectedStoreAdapters.tsservices/storage/ragVectorsProtectedStoreAdapter.tsservices/storage/storageEncryptionService.tstests/unit/ErrorBoundary.test.tsxtests/unit/hooks/useSettingsView.test.tstests/unit/settings/EncryptionRecoveryModal.test.tsxtests/unit/settings/PassphraseModal.test.tsxtests/unit/storage/primaryProtectedStoreAdapter.test.tstests/unit/storage/storageEncryptionService.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- components/settings/PrivacySection.tsx
- services/storage/ragVectorsProtectedStoreAdapter.ts
- components/settings/PassphraseModal.tsx
- services/storage/primaryProtectedStoreAdapter.ts
- docs/IDB-ENCRYPTION.md
- components/settings/EncryptionRecoveryModal.tsx
- App.tsx
- services/storage/primaryProtectedStoreAdapters.ts
- tests/unit/settings/PassphraseModal.test.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/osv-scanner.toml (1)
128-140: 🔒 Security & Privacy | 🔵 TrivialRun the Tauri bundle workflow after this change.
The
[[IgnoredVulns]]record is valid, andGHSA-jmr9-qjv8-65gvis the primary advisory ID forCVE-2026-56876. The file documents maintainer acceptance of the remaining ignore. Dispatch.github/workflows/tauri-build.ymland confirm Linux, Windows, and macOS bundles complete.🤖 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 `@src-tauri/osv-scanner.toml` around lines 128 - 140, Run the Tauri bundle workflow defined by tauri-build.yml after updating the IgnoredVulns entry, and verify that the Linux, Windows, and macOS bundle jobs complete successfully.Source: Coding guidelines
🤖 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 @.gitguardian.yaml:
- Around line 9-23: Remove the broad storageEncryptionService.test.ts entry from
secret.ignored-paths in the GitGuardian configuration. Replace the
false-positive fixture passphrases in the test with non-secret values where
possible, or add only their documented occurrences under secret.ignored_matches
using occurrence-scoped entries.
---
Nitpick comments:
In `@src-tauri/osv-scanner.toml`:
- Around line 128-140: Run the Tauri bundle workflow defined by tauri-build.yml
after updating the IgnoredVulns entry, and verify that the Linux, Windows, and
macOS bundle jobs complete successfully.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d3f44bc-1ecd-4fef-85c0-fc1f68a79725
📒 Files selected for processing (2)
.gitguardian.yamlsrc-tauri/osv-scanner.toml
Uh oh!
There was an error while loading. Please reload this page.
The IgnoredVulns entry itself was already added to src-tauri/osv-scanner.toml in an earlier commit on this branch; this adds the AUDIT.md documentation the repo's own housekeeping policy requires for accepted dependency risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
… commits Resolves overlapping edits between this stacked i18n branch and #342's own correction-loop work, which also touched locales/*/settings.json (both English source wording and several locales' translations for the encryption disable/rotate/recovery UI strings evolved independently after this branch diverged). Conflicts in the 19 locales/*/settings.json files were resolved with a scripted 3-way per-key JSON merge (git's textual merge conflates adjacent JSON lines into hunks even when only some individual keys actually conflict) using this policy: - Only one side changed a key from the merge base -> take that side. - Both changed it identically, or only one side has a real (non-English) translation -> take the real translation. - Both sides have differing real translations -> take #342's (verified this is not arbitrary: #342's non-English wording for every such key matches #342's own updated English source exactly, confirming its correction loop updated English + every translation together as one coherent unit, while this branch was simply stale relative to that whole update). - ar/he/fa (documented English-fallback RTL stubs) -> always track #342's newer English source on conflict. - The one key where this branch's content was genuinely unique and never touched by #342 (settings.privacy.encryptionDisableConfirm's fuller, more informative EN copy) was preserved. All 19 public/locales/*/bundle.json files were regenerated fresh via `pnpm run i18n:bundle` rather than hand-merged (2914 keys x 19 locales, clean). Every other locale module file (characters/common/outline/worlds/ writer/etc.) this branch touches had no overlap with #342 and merged automatically with no conflicts. All non-locale code/test/doc files from #342's correction loop also merged automatically with no conflicts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These 9 threads were still unresolved from an earlier review pass on this
branch, surfaced while preparing to merge all 5 open PRs:
- services/storage/storageEncryptionService.ts: fixed a real data-integrity
bug in resumeEncryptionMigration's committing-phase rekey recovery. The
try block wrapped deriveAndVerifySourceKeyFromSentinel,
deriveAndVerifyTargetKeyFromVerifier, AND commitRekeyMigration together, so
a commitRekeyMigration failure (after both passphrases had already verified
successfully) fell into the catch block, which then re-verified the target
passphrase, concluded "the commit must have already happened," and cleared
the journal — even though commitRekeyMigration had genuinely failed and the
durable sentinel could still hold the old passphrase while stores were
already re-encrypted under the new key. With no journal left, neither
passphrase could unlock the library. Isolated source-passphrase
verification into its own try/catch so only a genuine source-verify
failure enters the "maybe already committed" recovery branch; a
commitRekeyMigration failure now propagates as a retryable error with the
journal intact.
- tests/unit/storage/storageEncryptionService.test.ts: added the missing
hasPassphraseSentinel() assertion to the "rejects when neither passphrase
matches" test, matching its disable counterpart, so a regression that
clears the journal/sentinel on a failed recovery would fail this test.
Renamed all fixture passphrase literals ('original', 'new-pass',
'wrong-old', 'wrong-new') to an unambiguous 'test-fixture-*-passphrase'
pattern to reduce GitGuardian false-positive risk.
- .gitguardian.yaml: documented why the test file's path exclusion remains
(occurrence-scoped ignored_matches would be the narrower ideal, but
computing their SHA256 hashes requires the ggshield CLI + an authenticated
scan, unavailable in this environment) now that the fixture rename above
reduces the practical risk the exclusion covers.
- 7 locales (el, fi, fr, it, ko, pt, ru): fixed mistranslated or
inconsistent encryption-UI strings (Greek "disable" read as "make
incapable"; Korean used "unused" instead of "at rest" for encryption, and
a literal-translation "disable" verb instead of the idiomatic term; Russian
"disable" read as "forbid"; French/Italian's stuck-recovery message named
"the data" instead of "the migration" as unresumable; Finnish mixed
"salasana"/"tunnuslause" for passphrase inconsistently; Portuguese's AI
provider ready-status showed the infinitive "prepare" instead of "ready").
Rebuilt all 19 runtime bundles via `pnpm run i18n:check` (2914 keys x 19
locales, clean).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>qnbs
commented
Aug 13, 2026
OwnerAuthor
@coderabbitai review |
|
CI's doc-metrics drift gate (scripts/check-doc-metrics.mjs) caught this: the i18n additions from this branch's own correction-loop work plus the merged-in stacked i18n branch raised the key count from 2904 to 2914, but README.md's four "2904 keys" mentions (badge + 3 prose references) were never updated to match. Regenerated via `node scripts/sync-readme-metrics.mjs`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
qnbs
enabled auto-merge (squash)
August 13, 2026 03:19
Uh oh!
There was an error while loading. Please reload this page.
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
PR #342 was squash-merged into main, creating a new commit distinct from the individual commits this branch had already merged in from feat/encryption-migration-phase4-b. This produced a fresh set of textual conflicts in the 19 locales/*/settings.json files, but only ever the same single key already resolved once before: settings.privacy.encryptionDisableConfirm. This branch's own translation work (in all 19 languages, not just English) gives it a fuller, more informative warning than main's terse placeholder — kept this branch's version everywhere, matching the resolution already applied to this same key in the prior merge commit. Rebuilt all 19 public/locales/*/bundle.json bundles fresh via `pnpm run i18n:check` (2914 keys x 19 locales, clean) rather than resolving their conflicts by hand. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
…UI (#338) (#343) * feat(storage): wire encryption migration journal into production disable/rotate (#338 items 1+2) Replaces the clearIdbPassphrase/rotateIdbPassphrase stubs with real journal-backed migrations: new primary-store adapters (images, binder assets, codex, app-data, snapshots, RAG vectors) plug into the existing secondary-store adapters via a new encryptionMigrationOrchestrator.ts, giving the previously test-only migration engine its first production callers. Disable/rotate now migrate and verify every protected store before touching the passphrase sentinel, and fail closed into the resumable journal (not a partial verifier change) on error. Breaks a circular import (storageEncryptionService -> orchestrator -> adapters -> storageEncryptionService) via a dynamic import at the two call sites, matching this codebase's existing dynamic-import convention for avoiding heavy/circular static coupling (listenerMiddleware.ts, aiApi.ts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(settings): wire disable/rotate UI and startup recovery for at-rest encryption (#338 items 1+2) Extends PassphraseModal with 'disable'/'rotate' modes (live migration progress bar), adds Disable/Change-passphrase buttons to PrivacySection, and wires both through useSettingsView to the real clearIdbPassphrase/rotateIdbPassphrase implementations. Adds EncryptionRecoveryModal + an App.tsx startup check: if a disable/rotate journal was interrupted (reload/crash) before reaching 'completed', it takes priority over the normal unlock flow and lets the user re-enter their passphrase(s) to safely resume — CryptoKey material is never persisted, only re-derivable. A 'recovery-required' journal (the migration's own verification found an inconsistency) is surfaced as a distinct, honest stuck state rather than a fake auto-fix. Refactors clearIdbPassphrase/rotateIdbPassphrase's post-migration commit steps and key/verifier derivation into shared helpers so resumeEncryptionMigration() (used by the recovery UX) reaches the exact same end state as a fresh-start migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(storage,settings): cover the disable/rotate migration and recovery UX (#338) Real-data round-trip tests (image, codex 3-shape dispatch, binder asset Blob<->bytes, RAG vectors aggregate<->individual duality) through rekey then disable — the prior disable/rotate tests only proved migration completes over empty stores. Adds resumeEncryptionMigration coverage (correct resume, wrong source passphrase, missing target passphrase for rekey), new EncryptionRecoveryModal component tests, and disable/rotate mode + progress-bar tests for PassphraseModal, PrivacySection's new Change/Disable buttons, and useSettingsView's handlePassphraseConfirm branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: mark Phase 4 (#338) encryption disable/rotate/recovery as shipped Updates CLAUDE.md § At-rest encryption and Known Technical Debt, docs/IDB-ENCRYPTION.md, and ADR-0018's status line to reflect the production migration wiring implemented in this branch — the orchestrator, primary-store adapters, protectedWriteAdmission cross-tab gate, and EncryptionRecoveryModal. Remaining gap noted honestly: no E2E coverage yet for the disable/rotate/recovery round trips (unit + component tests only so far). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * i18n: add settings.privacy.* keys for the encryption disable/rotate/recovery UI (#338) 9 new keys (disable-failed, migration-progress + label, recovery title/body/stuck/ resume, changed-status) added to en/settings.json and propagated to all 19 locales via check-i18n-keys.mjs --fix. Real translations for the 5 production locales (en + de/es/fr/it); the remaining 14 carry the English fallback per this repo's tiered i18n convention. Rebuilds every public/locales/<lang>/bundle.json. Stacked on feat/encryption-migration-phase4-b to keep that PR's code-only diff under the ~100-file threshold the review bot needs to actually run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: update i18n key-count badge/table/prose to 2913 (was 2904) Fixes the doc-metrics drift check.mjs flagged after adding 9 settings.privacy.* keys for the encryption disable/rotate/recovery UI in the previous commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(security): gitleaks allowlist for PASSPHRASE_SENTINEL_RECORD_KEY false positive gitleaks' generic-api-key rule flags PASSPHRASE_SENTINEL_RECORD_KEY = 'idb_passphrase_sentinel_v1' (services/storage/idbPassphraseSentinel.ts, mirrored as a mock constant in storageEncryptionService.test.ts) purely because the variable name contains "PASSPHRASE" + "KEY" next to a sufficiently long, structured string. It's a static IndexedDB record-key identifier, not a credential — the app must know it deterministically to find the sentinel record via store.get/put/delete. Verified locally against the exact PR commit range with gitleaks 8.24.3 (matching CI): confirmed the finding reproduces without this config, and is suppressed with it, while an unrelated real-looking secret (Stripe key pattern) is still caught — the allowlist does not weaken detection elsewhere. No .gitleaks.toml existed before this; [extend] useDefault = true keeps every default rule active. Scoped to the exact leaked value only (not combined with a path filter) since gitleaks' top-level [allowlist] ORs paths/regexes rather than ANDing them — combining both would broaden the exemption to "anything in that file" instead of narrowing it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(storage): 3 critical encryption-migration bugs found in PR review Independently flagged by CodeAnt, CodeRabbit, and qodo-code-review on PR #342: 1. Disable/rekey recovery can permanently strand an interrupted migration. commitDisableMigration deletes the sentinel/salt BEFORE completing the journal; commitRekeyMigration saves the new sentinel BEFORE completing the journal. If completeEncryptionMigration() fails after either mutation, the journal stays 'committing' but resumeEncryptionMigration's source-key derivation now fails — the sentinel it needs is already gone (disable) or already replaced (rekey) — permanently blocking recovery. resumeEncryptionMigration now special-cases a journal already at 'committing': for disable, an absent sentinel means the commit already ran — finish the journal bookkeeping without deriving a key. For rekey, if the old passphrase no longer decrypts the sentinel, verify the new passphrase against the journal's durable target verifier (independent of the sentinel) before concluding the commit already happened, instead of reporting a false wrong-passphrase error. 2. Blob optimistic-write check only compared size + MIME type, not bytes. Two Blobs can share both while holding different content (e.g. a concurrent binder-asset re-upload), so the migration's freshness check would silently accept a stale value and overwrite a newer asset. valuesMatch is now async and compares actual bytes for Blobs. writeBatch is split into a read phase (readCurrentValues) and a write phase (putValues) so the async Blob comparison never happens inside a live IDB transaction's request handler — safe because the whole batch already runs under an exclusive withMigrationAdmission() lock, so no writer can interleave between the two. 3. The app-data primary-store adapter corrupted IdbKeyStore records. APP_DATA_STORE holds project/settings data AND IdbKeyStore's raw non-extractable CryptoKey (local_crypto_key_v2) plus per-provider encrypted API keys (api_key_<provider>_enc/_iv, legacy gemini_api_key_*). The adapter only reserved the migration journal + passphrase sentinel keys, so enabling/rotating/disabling encryption would JSON.stringify the CryptoKey (destroying it) and re-encrypt the already-encrypted API-key bytes as if they were plaintext project data — permanently losing every stored API key. idbKeyStore.ts now exports isKeyStoreRecordKey(), wired into the app-data adapter via a new isReservedKey predicate (reservedKeys alone can't express the dynamic per-provider key family). New regression coverage: 6 resumeEncryptionMigration committing-phase tests (both operations × not-yet-committed / partially-committed sub-states), a real disable+rekey round-trip proving a stored API key survives migration, and 11 direct unit tests for valuesMatch's Blob comparison + isKeyStoreRecordKey. All 98 pre-existing storageEncryptionService tests still pass unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(encryption): address remaining PR #342 review findings - ErrorBoundary: new `fallback` render-prop lets a boundary override the default non-blocking ErrorFallback; wired into App.tsx's EncryptionRecoveryModal boundary so a crash inside the recovery modal still renders a blocking, focus-trapped Modal instead of leaving the app underneath interactive during a pending disable/rekey recovery. - Condense every wrapped QNBS-v3 comment introduced this PR (App.tsx, EncryptionRecoveryModal.tsx) to the required single physical line. - docs/adr/0018: fix stale "disable/rekey rejected outright" and "cross-tab admission not shipped" claims to match the current implementation. - docs/IDB-ENCRYPTION.md: correct the protected-write call-path description — save paths snapshot the key via resolveProtectedWriteKey() then re-check only assertNoActiveEncryptionMigration() pre-transaction; delete paths call assertIdbProtectedWriteAllowed() directly. - tests/unit/storage/storageEncryptionService.test.ts: give every test a fresh IDBFactory instead of one shared factory for the whole file, via new _resetDbForTest()/_resetSentinelStoreForTest() exports alongside the existing __resetEncryptionMigrationJournalConnectionsForTest() — closes a latent cross-test leak where an encrypted record from one test could outlive its salt/sentinel into a later test. - tests/unit/hooks/useSettingsView.test.ts: add the rotate-failure coverage mirroring the existing disable-failure test. - tests/unit/ErrorBoundary.test.tsx: cover the new fallback prop. Assessed as already resolved by earlier commits in this PR (no further change needed, verified against current code): - PassphraseModal.tsx's rotate-mode catch already distinguishes a credential-mismatch IdbWrongPassphraseError (only ever thrown by the pre-migration old-passphrase check) from any other migration/journal failure, with test coverage for both paths. - primaryProtectedStoreAdapter.ts's verify()/migrateNext() both skip isReservedKey() records via readBatch(), so clearGeminiApiKey()/ clearApiKey() can no longer desync checkpoint.processed vs. verified — isKeyStoreRecordKey() already excludes them from migration scope entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): resolve OSV and GitGuardian findings on PR #342's branch - src-tauri/osv-scanner.toml: add the GHSA-jmr9-qjv8-65gv (extract-zip) ignore entry already present on the sibling fix/333-download-progress-metrics branch — no fixed version exists, transitive Playwright devDependency only. - .gitguardian.yaml: extend the existing "Generic Password" false-positive path exclusion to storageEncryptionService.test.ts, which pairs fake sourcePassphrase/targetPassphrase fixture literals ('original', 'new-pass', 'wrong-old', ...) exercising the disable/rotate/recovery flows against fake-indexeddb — same detector, same false-positive class already documented for the locale trees above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(i18n): translate the disable-encryption warning across all locales settings.privacy.encryptionDisableConfirm was updated in English to a detailed warning ("This will decrypt all your data and turn off at-rest encryption...") but the other 18 locales still carried the old short "Disable encryption" label. Since PassphraseModal renders this key as the disable-mode warning paragraph (not a button label), non-English users were seeing a misleading warning for a sensitive, irreversible-feeling action. Translated properly for all 18 remaining locales and rebuilt bundles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: document the extract-zip OSV ignore accepted-risk in AUDIT.md The IgnoredVulns entry itself was already added to src-tauri/osv-scanner.toml in an earlier commit on this branch; this adds the AUDIT.md documentation the repo's own housekeeping policy requires for accepted dependency risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address 9 remaining CodeRabbit findings on PR #342 These 9 threads were still unresolved from an earlier review pass on this branch, surfaced while preparing to merge all 5 open PRs: - services/storage/storageEncryptionService.ts: fixed a real data-integrity bug in resumeEncryptionMigration's committing-phase rekey recovery. The try block wrapped deriveAndVerifySourceKeyFromSentinel, deriveAndVerifyTargetKeyFromVerifier, AND commitRekeyMigration together, so a commitRekeyMigration failure (after both passphrases had already verified successfully) fell into the catch block, which then re-verified the target passphrase, concluded "the commit must have already happened," and cleared the journal — even though commitRekeyMigration had genuinely failed and the durable sentinel could still hold the old passphrase while stores were already re-encrypted under the new key. With no journal left, neither passphrase could unlock the library. Isolated source-passphrase verification into its own try/catch so only a genuine source-verify failure enters the "maybe already committed" recovery branch; a commitRekeyMigration failure now propagates as a retryable error with the journal intact. - tests/unit/storage/storageEncryptionService.test.ts: added the missing hasPassphraseSentinel() assertion to the "rejects when neither passphrase matches" test, matching its disable counterpart, so a regression that clears the journal/sentinel on a failed recovery would fail this test. Renamed all fixture passphrase literals ('original', 'new-pass', 'wrong-old', 'wrong-new') to an unambiguous 'test-fixture-*-passphrase' pattern to reduce GitGuardian false-positive risk. - .gitguardian.yaml: documented why the test file's path exclusion remains (occurrence-scoped ignored_matches would be the narrower ideal, but computing their SHA256 hashes requires the ggshield CLI + an authenticated scan, unavailable in this environment) now that the fixture rename above reduces the practical risk the exclusion covers. - 7 locales (el, fi, fr, it, ko, pt, ru): fixed mistranslated or inconsistent encryption-UI strings (Greek "disable" read as "make incapable"; Korean used "unused" instead of "at rest" for encryption, and a literal-translation "disable" verb instead of the idiomatic term; Russian "disable" read as "forbid"; French/Italian's stuck-recovery message named "the data" instead of "the migration" as unresumable; Finnish mixed "salasana"/"tunnuslause" for passphrase inconsistently; Portuguese's AI provider ready-status showed the infinitive "prepare" instead of "ready"). Rebuilt all 19 runtime bundles via `pnpm run i18n:check` (2914 keys x 19 locales, clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: sync README i18n key-count badges to 2914 (was stale at 2904) CI's doc-metrics drift gate (scripts/check-doc-metrics.mjs) caught this: the i18n additions from this branch's own correction-loop work plus the merged-in stacked i18n branch raised the key count from 2904 to 2914, but README.md's four "2904 keys" mentions (badge + 3 prose references) were never updated to match. Regenerated via `node scripts/sync-readme-metrics.mjs`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
Real conflicts (not just textual noise) resolved: - hooks/useSettingsView.ts + tests/unit/hooks/useSettingsView.test.ts: this branch's D5 useMemo-wrapped context-value fix was missing `migrationProgress` from its returned object — main's independently-added encryption feature had it, this branch's earlier refactor didn't carry it forward. Merged both: kept the useMemo optimization, added migrationProgress to the memoized object and its dependency array. Test mocks reconciled the same way — kept this branch's stable-toast pattern (the actual regression test for the D5 fix) while aliasing mockToastInfo/mockToastSuccess to stableToast's own methods so the existing encryption tests' assertions against those names keep working. - CHANGELOG.md: two genuinely distinct "### Fixed" entries from different PRs (#332's desktop boot/SettingsView fixes vs. #341/#344's AI Writing Studio readability fixes) — kept both. - README.md + locales/*/settings.json (el/fi/hu/is/pt/sv) + their bundles: mix of stale i18n key-count badges (regenerated via `node scripts/sync-readme-metrics.mjs` post-merge rather than guessing) and the same encryptionSetupFailed/providerStatusReady translation conflicts already resolved once in the previous merge commit — reapplied the same per-key resolution (whichever side has the real, non-English translation). Rebuilt all 19 bundles fresh via `pnpm run i18n:check` (2915 keys x 19 locales, clean) rather than resolving bundle.json conflicts by hand. - src-tauri/osv-scanner.toml: cosmetic-only comment-header conflict (both sides already had the identical extract-zip IgnoredVulns entry) — kept the more current review-date comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
Real conflicts resolved: - CHANGELOG.md: three genuinely distinct "### Fixed" entries from different PRs (#341/#344's AI Writing Studio readability fixes, already in main, vs. this branch's own voice-download-progress-scale fix) — kept both. - locales/pt/settings.json + its bundle: same providerStatusReady ("Preparar" infinitive vs. the already-fixed "Pronto") conflict already resolved on the other branches this session — took main's already-correct side, since this branch never touched that key. - README.md: stale i18n key-count/test-file-count badges — regenerated via `node scripts/sync-readme-metrics.mjs` post-merge rather than guessing (2918 keys × 19 locales, 547 test files). - src-tauri/osv-scanner.toml: cosmetic-only comment-header conflict (both sides already had the identical extract-zip IgnoredVulns entry) — kept the more current review-date comment. Rebuilt all 19 public/locales/*/bundle.json bundles fresh via `pnpm run i18n:check` rather than resolving bundle.json conflicts by hand. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5 tasks
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
…ontradictory local-only doc - CHANGELOG.md: the [1.27.0] section never documented its own headline feature (#342/#343's disable-encryption/passphrase-rotation production wiring) and still carried the prior release's "intentionally unavailable ... see issue #338" caveat, directly contradicting what shipped in this same version (chatgpt-codex-connector). Added the missing entry, qualified the stale caveat as describing that point in time, and corrected the Docs section's "opened issue #338" line to note it closed in this release. - docs/DEEPSOURCE-REVIEW-LOOP.md: two restored local-only tracking commits (originally discarded by an earlier `git reset --hard`, recovered via cherry-pick) carried "LOCAL-ONLY — not pushed to remote" / "do not push this entry upstream" wording that became false the moment they were committed to a pushed branch (qodo-code-review, coderabbitai). Removed the contradictory local-only framing, keeping the content as a normal dated log entry. Also softened the JS-0440 remediation TODO's "consider a rule-level ignore repo-wide" suggestion per coderabbitai's security concern — a blanket ignore would hide future unsafe `dangerouslySetInnerHTML` uses, not just the one reviewed occurrence. The public/sw.js QNBS-v3-comment findings (qodo-code-review, coderabbitai) were false positives — the existing single-line QNBS-v3 comment on the line directly above APP_VERSION was already unchanged by this PR's one-line version-bump diff; verified via `git diff main~1 -- public/sw.js`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qnbs added a commit
that referenced
this pull request
Aug 13, 2026
…reliability fixes (#351) * docs(deepsource): log re-surfaced JS-0440 finding (local-only, not for push) Co-Authored-By: GitHub Copilot (Claude Sonnet 5) <noreply@github.com> * docs(deepsource): correct stale JS-0440 disposition, re-surfaced 2026-08-01 (local-only) Co-Authored-By: GitHub Copilot (Claude Sonnet 5) <noreply@github.com> * release: v1.27.0 — Phase 4 encryption production wiring + desktop/AI reliability fixes Closes out issue #338 (Phase 4 of the at-rest encryption lifecycle): disable encryption and passphrase rotation are now live in Settings › Privacy, backed by the durable resumable migration journal built in the prior release. Also ships three independently-diagnosed reliability/UX fixes: - Tauri desktop cold boot never read persisted state back (#332) — every desktop launch loaded as a brand-new user regardless of what was actually saved to disk; boot hydration now mirrors the already-correct save path. Quitting also now awaits any pending debounced autosave instead of risking a mid-debounce data loss. - AI Writing Studio manuscript text was unreadable, with the caret/selection visually drifting from the real text (#341) — a blur/font-mismatch/ scroll-desync defect in the invisible-input-over-visible-mirror rendering pattern used by both Writer Studio and the main manuscript editor. - Voice and WebLLM model download progress bars showed real byte counts and transfer speed instead of a bare percentage, and the voice download bar's progress-scale bug (stuck at ~95% for most of the download) is fixed (#333 item 1). All 5 correction-loop PRs (#342-#346) ran to full quiescence before merging — every CodeRabbit/CodeAnt/Qodo/Sourcery finding fixed or justified with evidence, 0 unresolved review threads, full CI green (Quality Gate, E2E, E2E Deep Coverage, Build, Storybook, Lighthouse, Visual Regression) — including a genuine data-integrity bug found and fixed during that loop: a shared try/catch in the rekey-resume recovery path could, on a crash immediately after `commitRekeyMigration`, misinterpret that crash as "already committed" and clear the migration journal while the durable sentinel still held the old passphrase — leaving neither passphrase able to unlock the library. 2919 i18n keys × 19 locales. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address PR #351 review findings — stale CHANGELOG claim + self-contradictory local-only doc - CHANGELOG.md: the [1.27.0] section never documented its own headline feature (#342/#343's disable-encryption/passphrase-rotation production wiring) and still carried the prior release's "intentionally unavailable ... see issue #338" caveat, directly contradicting what shipped in this same version (chatgpt-codex-connector). Added the missing entry, qualified the stale caveat as describing that point in time, and corrected the Docs section's "opened issue #338" line to note it closed in this release. - docs/DEEPSOURCE-REVIEW-LOOP.md: two restored local-only tracking commits (originally discarded by an earlier `git reset --hard`, recovered via cherry-pick) carried "LOCAL-ONLY — not pushed to remote" / "do not push this entry upstream" wording that became false the moment they were committed to a pushed branch (qodo-code-review, coderabbitai). Removed the contradictory local-only framing, keeping the content as a normal dated log entry. Also softened the JS-0440 remediation TODO's "consider a rule-level ignore repo-wide" suggestion per coderabbitai's security concern — a blanket ignore would hide future unsafe `dangerouslySetInnerHTML` uses, not just the one reviewed occurrence. The public/sw.js QNBS-v3-comment findings (qodo-code-review, coderabbitai) were false positives — the existing single-line QNBS-v3 comment on the line directly above APP_VERSION was already unchanged by this PR's one-line version-bump diff; verified via `git diff main~1 -- public/sw.js`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: GitHub Copilot (Claude Sonnet 5) <noreply@github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
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 freeto 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.
User description
Summary
Completes issue #338's Phase 4 by wiring the previously test-only encryption migration
journal into real production entry points for disabling at-rest encryption and rotating
the passphrase. Stacked on #339 (merged), which already fixed the write-vs-migration race
this PR's migrations depend on.
services/storage/primaryProtectedStoreAdapter.ts+primaryProtectedStoreAdapters.ts+ragVectorsProtectedStoreAdapter.ts): images,binder assets (Blob↔bytes), codex (3-shape dispatch), app-data/settings, snapshots
(generic explicit-key/inline-keyPath engine), and a bespoke per-project RAG-vector
adapter (aggregate ↔ individual-record duality).
services/storage/encryptionMigrationOrchestrator.ts) combinesthe new primary adapters with the pre-existing secondary-store adapters
(scene revisions, inference cache) into one journal-backed migration run, with a
progress callback threaded through
protectedStoreMigration.ts.clearIdbPassphrase()/rotateIdbPassphrase()instorageEncryptionService.tsreplace the stubs that always threw
IdbEncryptionMigrationRequiredError— each nowmigrates + verifies every protected store before touching the passphrase sentinel, and
leaves the durable journal in place (not a partial verifier change) on failure.
PassphraseModalgains'disable'/'rotate'modes with a live migrationprogress bar;
PrivacySectiongains Change-passphrase/Disable buttons.EncryptionRecoveryModal+ an App.tsx startup check — if adisable/rotate migration was interrupted (reload/crash) before reaching
'completed',it takes priority over the normal unlock flow and lets the user re-enter their
passphrase(s) to resume via
resumeEncryptionMigration(). A'recovery-required'journal (the migration's own verification found an inconsistency) is surfaced as an
honest stuck state, not auto-fixed.
storageEncryptionService→ orchestrator → adapters →storageEncryptionService) via a dynamic import at the two call sites, matching thisrepo's existing convention (
listenerMiddleware.ts,aiApi.ts).Stacked PR: i18n key additions (9 new
settings.privacy.*keys × 19 locales +bundle rebuild, 144 files) are in a follow-up PR based on this branch, kept separate so
CodeAnt's ~100-file review threshold still fires on both.
Test plan
pnpm run typecheck— cleanpnpm run lint— cleanBlob↔bytes, RAG-vector aggregate↔individual) through rekey then disable
resumeEncryptionMigrationcoverage: correct resume, wrong source passphrase,missing target passphrase for rekey
EncryptionRecoveryModalcomponent tests (11)PassphraseModal/PrivacySection/useSettingsViewdisable/rotate + progress-bartests
Closes#338 items 1 & 2 (disable/rotate production flow); items 3 & 4 already closed
by #339.
🤖 Generated with Claude Code
Summary by Sourcery
Wire production disable and passphrase rotation flows for IndexedDB at-rest encryption, including migration orchestration, UI, and recovery UX.
New Features:
Enhancements:
Documentation:
Tests:
CodeAnt-AI Description
Enable production encryption disable, passphrase rotation, and migration recovery
What Changed
Impact
✅ Passphrase rotation without losing protected project data✅ Safe encryption disable after full data verification✅ Resumable recovery after interrupted migrations💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests