Repository navigation
Conversation
Builds before vault versioning upsert their vault_meta row on schema_version = 1, so recording the version there broke their init. Keep schema_version at 1, record vault_version and min_vault_version in their own columns, and open newer vaults without migrating unless they require a newer build. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Dzejkop
requested review from
a team,
Guardiola31337,
danielle-tfh and
wld-walletkit-bot
as code owners
October 29, 2026 16:30
Contributor
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Version metadata and schema validation crates/walletkit-core/src/storage/credential_vault/schema.rs, crates/walletkit-core/src/storage/credential_vault/fixtures/v1.sql |
Vault metadata tracks the applied migration version and minimum compatible version separately while keeping schema_version at 1. Validation checks stored values, required tables, and index layouts. The fixture defines metadata and credential tables with their indexes and update trigger. |
Migration and compatibility handling crates/walletkit-core/src/storage/credential_vault/schema.rs |
Migrations record versions transactionally and preserve higher stored values. Legacy schemas can migrate to the current layout. A vault with a higher applied version opens when its minimum version is supported. Tests cover data preservation, rollback and retry, concurrent migration, and version validation. |
Vault opening and leaf-index initialization crates/walletkit-core/src/storage/credential_vault/mod.rs, crates/walletkit-core/src/storage/credential_vault/schema.rs |
CredentialVault::new passes ensure_schema to Vault::open. Leaf-index initialization updates a single metadata row, rejects a mismatched index, and returns an invalid-row-count error when the row count is zero or greater than one. Tests cover initialization and compatibility with older metadata layouts. |
Priority:
Estimated code review effort: 3 (Moderate) | ~30 minutes
Change: Bug fix
Suggested reviewers: kilianglas
Merge Risk: 
a180b
-
No concrete compatibility or runtime issue remains identified; the PR is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title says the PR adds vault schema migrations that older builds can read. The changeset matches this. schema.rs adds vault_version and min_vault_version tracking, migration v2 converts #632-layou…Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title says the PR adds vault schema migrations that older builds can read. The changeset matches this. schema.rs adds vault_version and min_vault_version tracking, migration v2 converts
#632-layoutvaults back to the legacy index, and older builds keep schema_version at 1 with its unique index. The title names the main change, and it is specific to the storage layer. The title is one sentence and 69 characters, which fits the length guidance.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 20 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
Comment @coderabbitai help to get the list of available commands.
Collaborator
Agent review · Approval withheldHead, base, policy, or review evidence changed during review Reviewed commit: Agent recommendation: approve Approved. The required CodeRabbit and Copilot reviews cover the current content, the single review finding is fixed and confirmed, and my independent review found no blocking defects. CodeRabbit approved the exact head SHA (review 5710525316, 2026-10-30T09:42:12Z). Copilot recommended approval on commit 6cb65e60; I verified that 6cb65e60 and head a180bac2 share tree 6d3281d4efb3543e293fd1e6d1e3526a0d89052a (git diff 6cb65e60..a180bac2 is empty), so head a180bac2 is a merge of main that introduces no content Copilot did not review. The only remaining automated note is CodeRabbit's non-blocking docstring-coverage warning on the initial commit, which did not prevent CodeRabbit from approving the head. No unresolved concerns, change requests, or unanswered review threads remain. Final agent responseVerdict written to `/home/runner/work/_temp/review/verdict.json`. **Decision: approve.** Key points: - **Coverage:** CodeRabbit approved the exact head `a180bac2`. Copilot's latest review (approval recommended, no open issues) is on `6cb65e6`, whose tree is byte-identical to the head (`6d3281d4…`, empty `git diff`), so the merge of main introduced no unreviewed content. CI on `6cb65e6` passed. - **Discussion:** The single Copilot P2 (concurrent legacy version/index snapshot) is resolved and outdated; I verified the fix in `read_legacy_version` reads `schema_version` and the singleton-index existence in one statement. - **Independent review:** Traced migrations and version handling across fresh, legacy, #632, stale, current, and newer vaults; transactional rollback, atomic version recording, `schema_version = 1` + unique index compatibility with v0.28.0, and backup exclusion of `vault_meta` all check out. No blocking defects. - **Policy:** Open, non-draft, in-repo, into main; no protected-file changes; no change requests or unresolved threads; format-sensitive work guarded by the frozen `v1.sql` fixture. No blockers. Structured review assessmentReview coverageCodeRabbit: APPROVED on head a180bac2 (review 5710525316). Its summary reported no actionable comments and its only historical concern was the concurrency issue, now fixed. Copilot: latest review 5701620393 on commit 6cb65e60 is 'Approval recommended' with no open issues; earlier review 5701358246 raised the single P2 finding. Because 6cb65e6 and a180bac2 have byte-identical trees, Copilot's review covers exactly the head content. Depth is appropriate: both bots reviewed the full migration implementation and its large test suite, and the harness confirms CI (run 40915078144 for 6cb65e6, same tree) completed successfully, including the crate tests, Clippy/format, Swift, Kotlin, WASM, and web checks. The only pending checks are the same CI jobs re-running for the merge commit; the content is unchanged. This satisfies the policy's requirement for completed CodeRabbit and Copilot reviews over the current changes. Discussion resolutionOne review thread exists (PRRT_kwDOLGbMQc6xptfS, schema.rs), marked resolved and outdated. Copilot's [P2] finding was that concurrent initialization of a #632-layout vault could read schema_version=2 in one snapshot and then check the singleton index in a later snapshot, transiently rejecting the vault as 'invalid vault version'. The head code fixes this: read_legacy_version evaluates schema_version and EXISTS(idx_vault_meta_singleton) in a single SELECT over vault_meta, so both values come from one snapshot, and the pre-lock path performs no separate index check (migrate rechecks everything under the write lock in transaction_immediate). I confirmed this in the head diff; resolved/outdated status is backed by the code, not merely the flag. Copilot's follow-up review lists the issue as resolved. No other inline threads or general findings (the two CodeRabbit auto-fix completion comments are unrelated to this PR's code). Independent reviewI read .code-review.md, AGENTS.md, evidence.json, and the full diff for the three changed files (fixtures/v1.sql added; mod.rs; schema.rs). I traced ensure_schema, migrate, read_version, read_legacy_version, validate_schema, validate_legacy_schema, record_version, migration_v1/v2, and init_leaf_index across fresh, legacy-v1 (initialized and uninitialized), #632 singleton-index, current, stale-version, and newer-than-supported vaults. Migrations run inside transaction_immediate with an under-lock recheck, record the version atomically with MAX(), and roll back DDL and version on failure. schema_version stays 1 with the unique idx_vault_meta_schema_version, so v0.28.0-and-earlier's ON CONFLICT(schema_version) upsert keeps working; a test replays that pre-vault_version SQL against a migrated vault. vault_meta remains excluded from BACKUP_TABLES, and credential_records/blob_objects DDL and compute_content_id are untouched. Incomplete schemas and missing/duplicate metadata error instead of being reset. init_leaf_index now reads COUNT(*) and MAX(leaf_index) in one snapshot and errors on anomalous row counts rather than upserting. I noted two intentional behavior changes (blob_objects and dropped metadata indexes are no longer silently recreated for current vaults, and forward versions must retain the legacy index for version>=2); both are guarded by explicit errors, covered by tests, and consistent with the stated compatibility design, not defects. The frozen v1.sql fixture guards the on-disk legacy layout. No blocking defect found. Repository policyAll repository approval conditions in .code-review.md are met. PR #637 is open, non-draft, from this repository (author Dzejkop), into main, with no no-auto-approve label and no changes to .github/ or review instruction files (only the three credential_vault files). The required reviewers are covered as described above. No outstanding CHANGES_REQUESTED reviews and no unresolved threads. AGENTS.md compatibility expectations hold: schema_version remains 1 with its unique index, CBOR layouts and compute_content_id are unchanged, and the format-sensitive migration is guarded by a frozen v1.sql fixture plus legacy-reopen/backup-import tests. Logging/error messages across the FFI boundary carry only generic text and integer version numbers, so no sensitive user data leaks. The only non-blocking caveat is CodeRabbit's docstring-coverage warning, which it accepted by approving the head. verdict.json{
"head": "a180bac2f916d74dfc78391c17a80688f7b9add6",
"evidence": "2ae2e25302c845b8966668ebabed8297598d297816f390b3e5a683384da836a7",
"approve": true,
"reason": "Approved. The required CodeRabbit and Copilot reviews cover the current content, the single review finding is fixed and confirmed, and my independent review found no blocking defects. CodeRabbit approved the exact head SHA (review 5710525316, 2026-10-30T09:42:12Z). Copilot recommended approval on commit 6cb65e60; I verified that 6cb65e60 and head a180bac2 share tree 6d3281d4efb3543e293fd1e6d1e3526a0d89052a (git diff 6cb65e60..a180bac2 is empty), so head a180bac2 is a merge of main that introduces no content Copilot did not review. The only remaining automated note is CodeRabbit's non-blocking docstring-coverage warning on the initial commit, which did not prevent CodeRabbit from approving the head. No unresolved concerns, change requests, or unanswered review threads remain.",
"review_coverage": "CodeRabbit: APPROVED on head a180bac2 (review 5710525316). Its summary reported no actionable comments and its only historical concern was the concurrency issue, now fixed. Copilot: latest review 5701620393 on commit 6cb65e60 is 'Approval recommended' with no open issues; earlier review 5701358246 raised the single P2 finding. Because 6cb65e6 and a180bac2 have byte-identical trees, Copilot's review covers exactly the head content. Depth is appropriate: both bots reviewed the full migration implementation and its large test suite, and the harness confirms CI (run 40915078144 for 6cb65e6, same tree) completed successfully, including the crate tests, Clippy/format, Swift, Kotlin, WASM, and web checks. The only pending checks are the same CI jobs re-running for the merge commit; the content is unchanged. This satisfies the policy's requirement for completed CodeRabbit and Copilot reviews over the current changes.",
"discussion_resolution": "One review thread exists (PRRT_kwDOLGbMQc6xptfS, schema.rs), marked resolved and outdated. Copilot's [P2] finding was that concurrent initialization of a #632-layout vault could read schema_version=2 in one snapshot and then check the singleton index in a later snapshot, transiently rejecting the vault as 'invalid vault version'. The head code fixes this: read_legacy_version evaluates schema_version and EXISTS(idx_vault_meta_singleton) in a single SELECT over vault_meta, so both values come from one snapshot, and the pre-lock path performs no separate index check (migrate rechecks everything under the write lock in transaction_immediate). I confirmed this in the head diff; resolved/outdated status is backed by the code, not merely the flag. Copilot's follow-up review lists the issue as resolved. No other inline threads or general findings (the two CodeRabbit auto-fix completion comments are unrelated to this PR's code).",
"independent_review": "I read .code-review.md, AGENTS.md, evidence.json, and the full diff for the three changed files (fixtures/v1.sql added; mod.rs; schema.rs). I traced ensure_schema, migrate, read_version, read_legacy_version, validate_schema, validate_legacy_schema, record_version, migration_v1/v2, and init_leaf_index across fresh, legacy-v1 (initialized and uninitialized), #632 singleton-index, current, stale-version, and newer-than-supported vaults. Migrations run inside transaction_immediate with an under-lock recheck, record the version atomically with MAX(), and roll back DDL and version on failure. schema_version stays 1 with the unique idx_vault_meta_schema_version, so v0.28.0-and-earlier's ON CONFLICT(schema_version) upsert keeps working; a test replays that pre-vault_version SQL against a migrated vault. vault_meta remains excluded from BACKUP_TABLES, and credential_records/blob_objects DDL and compute_content_id are untouched. Incomplete schemas and missing/duplicate metadata error instead of being reset. init_leaf_index now reads COUNT(*) and MAX(leaf_index) in one snapshot and errors on anomalous row counts rather than upserting. I noted two intentional behavior changes (blob_objects and dropped metadata indexes are no longer silently recreated for current vaults, and forward versions must retain the legacy index for version>=2); both are guarded by explicit errors, covered by tests, and consistent with the stated compatibility design, not defects. The frozen v1.sql fixture guards the on-disk legacy layout. No blocking defect found.",
"policy_checks": "All repository approval conditions in .code-review.md are met. PR #637 is open, non-draft, from this repository (author Dzejkop), into main, with no no-auto-approve label and no changes to .github/ or review instruction files (only the three credential_vault files). The required reviewers are covered as described above. No outstanding CHANGES_REQUESTED reviews and no unresolved threads. AGENTS.md compatibility expectations hold: schema_version remains 1 with its unique index, CBOR layouts and compute_content_id are unchanged, and the format-sensitive migration is guarded by a frozen v1.sql fixture plus legacy-reopen/backup-import tests. Logging/error messages across the FFI boundary carry only generic text and integer version numbers, so no sensitive user data leaks. The only non-blocking caveat is CodeRabbit's docstring-coverage warning, which it accepted by approving the head."
}
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent vault initialization can read schema metadata and index state from different snapshots, incorrectly rejecting a migrated vault with a transient invalid vault version error.
1 open issue
What changed in this PR
This PR preserves vault compatibility with released WalletKit builds, supporting app rollback and shared-vault access across versions.
Changes:
- Keeps
schema_version = 1and tracks migrations and compatibility in separate columns. - Allows compatible newer vaults to open without migration and repairs the metadata layout introduced by #632.
- Adds compatibility tests and makes leaf-index initialization rely on schema-created metadata.
| File | Description |
|---|---|
crates/walletkit-core/src/storage/credential_vault/schema.rs |
Updates version tracking, compatibility checks, migration repair, and regression tests. |
crates/walletkit-core/src/storage/credential_vault/mod.rs |
Removes metadata insertion from leaf-index initialization. |
Review effort: Balanced
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Read the legacy marker and single-row index in one statement so a concurrent conversion cannot be rejected, make migration v2 safe to reapply, and cover released-build init after converting a vault. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
|
Auto-fix stopped before satisfying the configured readiness conditions. Blocking condition: Auto-fix requires data retention to be enabled for the organization. Triggered by rule Auto-fix PRs. This is the Auto-fix result for commit 6cb65e6; it does not confirm that the pull request was merged. |
wld-walletkit-bot
previously approved these changes
Oct 29, 2026
wld-walletkit-bot
left a comment
wld-walletkit-bot
left a comment
Collaborator
There was a problem hiding this comment.
Agent review passed the repository review policy, discussion resolution, and independent code review. Verdict and evidence: https://github.com/worldcoin/walletkit/actions/runs/40953333916
Contributor
|
Auto-fix stopped before satisfying the configured readiness conditions. Blocking condition: Auto-fix requires data retention to be enabled for the organization. Triggered by rule Auto-fix PRs. This is the Auto-fix result for commit 6cb65e6; it does not confirm that the pull request was merged. |
Keep this branch's vault schema: it carries #632's migrations with the vault_version layout.
Dzejkop
dismissed stale reviews from wld-walletkit-bot and coderabbitai[bot]
October 30, 2026 09:38
The merge-base changed after approval.
Dzejkop
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds transactional schema migrations to the credential vault, and keeps migrated vaults usable by released builds. This replaces #632, which was reverted in #639.
Released builds (v0.28.0 and earlier) have no version checks. On every start they upsert their
vault_metarow withON CONFLICT(schema_version), so the layout must keep that working:schema_versionstays 1 and keeps its unique index. That still guarantees a single metadata row.vault_versionrecords the applied migrations, andmin_vault_versionthe oldest vault version whose code can use the vault.min_vault_versionis newer than the build. That allows an app rollback, and an older companion app sharing the vault on iOS.Validation:
Stack parent of #633.
AI: Claude Opus 5.5 implementation. Prompt: add vault schema migrations that keep migrated vaults readable by older builds and allow rollback.
🤖 Generated with Claude Code
Note
High Risk
Changes encrypted credential vault schema, migration, and account metadata initialization—core persistence that must stay compatible across app versions and shared-vault scenarios.
Overview
Fixes credential vault upgrades so app rollback and older WalletKit builds (e.g. v0.28.0) can still open a vault after migration, instead of hitting unique-constraint errors on
vault_meta.Schema versioning replaces the one-shot DDL with ordered migrations (
migration_v1/migration_v2). Applied version is stored in newvault_versionandmin_vault_versioncolumns;schema_versionstays fixed at 1 with the existing unique index on it. Migration v2 also converts vaults from the prior #632 layout (singleton index / version inschema_version) back to that legacy shape. Opens validate completeness, reject unsupportedmin_vault_version, run pending migrations in a singletransaction_immediate, and warn (without migrating) when the stored version is newer than the app.init_leaf_indexno longer upserts metadata rows; it requires exactly one metadata row and updatesleaf_indexwhen unset, so older releases can keep using theirON CONFLICT(schema_version)init path on the same database.Adds a v1 SQL fixture and a large migration/rollback/concurrency test suite (legacy adoption, failed migration rollback, backup import/merge, pre-
vault_versioninit SQL).Reviewed by Cursor Bugbot for commit a180bac. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit