[Claudius-Maginificent]

Issue being fixed or feature implemented

Several open PRs add SQLite wallet-storage migrations in parallel and pick version numbers independently (e.g. two V022s, V019s that collide with base). Refinery's default abort_missing refuses to open a database whose refinery_schema_history already holds a higher version than an unapplied embedded migration. CI only migrates fresh databases, so a migration slotted below an already-shipped version passes every test and then fails at wallet open on upgraded installs.

What was done?

Added a unit test, embedded_migration_versions_are_unique_and_contiguous, that requires the embedded migration versions to be exactly 1..=N. A new migration must therefore take "highest version on the base branch + 1". A reused number or a gap fails as soon as the branch is merged with its base, with a message that says to renumber. No new CI job: the test runs with the existing platform-wallet-storage suite.

How Has This Been Tested?

• cargo test -p platform-wallet-storage --lib embedded_migration_versions: passes on current v5.1-dev (V001..V019).
• Negative check: renaming V019__asset_lock_operation_id.rs to V021__... makes the test fail with the renumber message (reverted).
• cargo clippy -p platform-wallet-storage --all-targets -- -D warnings: clean.

Breaking Changes

None. Open PRs whose migration number leaves a gap (currently #5479 with V022) will need to renumber once they merge this base.

Checklist:

• I have performed a self-review of my own code
• My Rust, FFI, WASM, Swift or Kotlin changes follow the coding conventions
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed
• If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 09ab188

• Bots — coderabbitai skipped by lklimek · thepastaclaw ✓
• Self-review — not asked of a bot author
• Build green
• Approvals
  • wallet-storage (packages/rs-platform-wallet-storage/src/sqlite/migrations.rs) — approved by lklimek

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Source mode: Auto
• Repository: dashpay/platform
• Branch: test/wallet-storage-migration-versions
• File: .coderabbit.yaml
• Commit: 09ab188
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8319fea2-1d1c-4e9e-a5f9-22f895181fbb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

✅ Final review complete — no blockers (commit 09ab188) · triage: low

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The migration-version test correctly enforces uniqueness and contiguity, and the wallet test job covers it. Two documentation comments are warranted: SCHEMA.md now understates the number of guards and references a renamed test, while the new doc comment inaccurately describes CI as migrating only fresh databases. The remaining co-location concern is a minor discoverability nitpick, not a correctness issue.

🟡 1 suggestion(s) | 💬 2 nitpick(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Update the migration guard documentation for the new test
packages/rs-platform-wallet-storage/SCHEMA.md:814-817

This paragraph is now inaccurate after adding embedded_migration_versions_are_unique_and_contiguous: it still says there are only two guards and refers to tc_b_031, a test name that no longer exists. Update it to document all three guards and reference v4_2_dev_database_opens_and_migrates_forward, so the migration-number policy remains discoverable and the documentation does not contain a dangling test name.

Three guards enforce it: `merged_migration_versions_keep_their_shipped_names` pins the version-to-name bindings, `v4_2_dev_database_opens_and_migrates_forward` opens a database created by `v4.2-dev`'s own binary (`tests/fixtures/v4_2_dev_migrated.db`) and migrates it the whole way forward, and `embedded_migration_versions_are_unique_and_contiguous` requires the embedded versions to be exactly `1..=N`, so new work takes the highest version on the base branch + 1.

_{source: glm-5.3-flash (phase1-reviewer: general, rust-quality)}

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The diff adds one small, straightforward unit test enforcing migration-version uniqueness and contiguity without changing production behavior or migrations.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — rust-quality (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 91% left, weekly 81% left; passed over gemini-3.8-flash-high (antigravity quota lookup failed)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/SCHEMA.md`:
- [SUGGESTION] packages/rs-platform-wallet-storage/SCHEMA.md:814-817: Update the migration guard documentation for the new test
  This paragraph is now inaccurate after adding `embedded_migration_versions_are_unique_and_contiguous`: it still says there are only two guards and refers to `tc_b_031`, a test name that no longer exists. Update it to document all three guards and reference `v4_2_dev_database_opens_and_migrates_forward`, so the migration-number policy remains discoverable and the documentation does not contain a dangling test name.

In `packages/rs-platform-wallet-storage/src/sqlite/migrations.rs`:
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/migrations.rs:428-435: Clarify the migration test's CI coverage comment
  The comment says CI only migrates fresh databases, but the integration suite also migrates the committed `v4_2_dev_migrated.db` fixture, whose history reaches V006. The important limitation is that no CI database currently has history above the versions needed to expose a migration slotted below a newer shipped version; also, a plain two-file duplicate may surface only as an opaque primary-key error after merge. Tightening the wording will keep the rationale accurate.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/migrations.rs:436-437: Cross-reference the migration-set guard with the sibling schema pins
  The new guard validates the same embedded migration set as the set-level checks in `tests/integration/sqlite_schema_pinning.rs`, but it is separated from those checks because the unit-test location works without the integration test feature gate. That placement is valid, but a short cross-reference would make the relationship discoverable when maintainers update the migration fingerprints or add a migration.

[lklimek]

/skip-bots

[github-actions]

Policy satisfied — this can merge.
Full checklist in the description.