fix(db): resolve V15 migration numbering conflict

[ilblackdragon]

Summary

• Renumber PostgreSQL migrations to resolve duplicate V15: user_identities stays V15, conversation_source_channel becomes V16, document_versions becomes V17 — matching the libSQL incremental ordering
• Add user_identities, memory_document_versions, and source_channel column to the libSQL base schema so fresh databases get all tables
• Add repair_misnumbered_v15() to fix existing databases where V15 was recorded as "document_versions" instead of "user_identities", causing no such table: user_identities errors
• Add regression test test_v15_misnumbered_repair

Test plan

• cargo check --no-default-features --features libsql — compiles clean
• cargo clippy --no-default-features --features libsql --lib -- -D warnings — zero warnings
• Identity CRUD tests pass (test_identity_crud, test_find_by_verified_email_ignores_unverified, test_create_user_with_identity)
• Regression test test_v15_misnumbered_repair passes
• Verify on a database with the stale V15 record that restart creates the user_identities table

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request updates the database schema by adding a source_channel column to the conversations table and introducing new tables for user_identities and memory_document_versions. It also implements a repair mechanism, repair_misnumbered_v15, to address a regression where migration V15 was incorrectly recorded, and includes a corresponding regression test. The review feedback highlights that renaming PostgreSQL migration files can lead to checksum mismatches and suggests using .transpose() instead of .ok().flatten() when fetching rows to ensure database errors are not silently ignored.

[gemini-code-assist]

While this repair logic correctly handles the renumbering for libSQL, the PR also renames the PostgreSQL migration files (e.g., V15 to V16). Renaming migrations can cause checksum mismatches or duplicate execution attempts on existing databases. If PostgreSQL is a supported production backend, consider whether a similar repair or a manual migration guide is needed. Additionally, prioritize consistency over refactoring pre-existing code outside the scope of the fix, and ensure migrations are wrapped in atomic transactions.

References

1. Ensure database migrations and the recording of their completion are wrapped in a single atomic transaction to prevent inconsistent states and allow for safe retries.
2. For bugfixes, prioritize consistency with existing code patterns over refactoring to reduce duplication, especially if the refactor would touch pre-existing code outside the scope of the fix.

[ilblackdragon]

The PostgreSQL migration renames are safe here — V15__conversation_source_channel.sql was introduced in a branch that conflicted with V15__user_identities.sql, and neither had been deployed to any PostgreSQL database yet (they landed simultaneously via the same merge). Refinery tracks by version number and the SQL content is identical, just at new version numbers. No existing PG database has a V15 or V16 that would mismatch.

[gemini-code-assist]

The use of .ok().flatten() on the result of rows.next().await silently swallows any database errors that might occur during row fetching. This could lead to the repair logic being skipped without any indication of failure. It's better to handle the error explicitly using transpose(). Furthermore, ensure that database migrations and the recording of their completion are wrapped in a single atomic transaction to prevent inconsistent states.

    if let Some(row_res) = rows.next().await.transpose() {
        let row = row_res.map_err(|e| {
            DatabaseError::Migration(format!("V15 repair: failed to fetch row: {e}"))
        })?;
        let name: String = row.get(0).map_err(|e| {
            DatabaseError::Migration(format!("V15 repair: failed to read name: {e}"))
        })?;

References

1. Ensure database migrations and the recording of their completion are wrapped in a single atomic transaction to prevent inconsistent states and allow for safe retries.

[ilblackdragon]

Good catch — replaced .ok().flatten() with explicit .map_err()? so DB errors during the repair check are properly surfaced instead of silently skipped. Fixed in 64d5752.

[Copilot]

Pull request overview

Resolves a PostgreSQL migration numbering collision around V15+ while keeping libSQL and Postgres schemas aligned, and adds a targeted libSQL repair for an earlier mis-recorded migration entry.

Changes:

• Renumbers PostgreSQL migrations so conversation_source_channel is V16 and document_versions is V17 (leaving user_identities as V15).
• Updates the libSQL base schema to include source_channel, user_identities, and memory_document_versions for fresh databases.
• Adds a libSQL repair step for databases that recorded V15 under the wrong name, plus a regression test.

Reviewed changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/db/libsql/identities.rs	Adds regression test to ensure misnumbered V15 state is repaired by libSQL migrations.
src/db/libsql_migrations.rs	Extends libSQL base schema and adds repair_misnumbered_v15() invoked before incremental migrations.
migrations/V16__conversation_source_channel.sql	Adds source_channel column to Postgres conversations table.
migrations/V17__document_versions.sql	Adds Postgres memory_document_versions table (and an additional metadata GIN index).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

repair_misnumbered_v15 currently deletes the V15 row whenever the recorded name is anything other than "user_identities". That’s broader than the stated repair (a specific earlier release recorded V15 as "document_versions") and can unexpectedly rewrite migration history if the name differs for any other reason. Consider tightening the condition to the known bad value ("document_versions") and/or checking whether the user_identities table exists before deleting; alternatively, update the doc/comment to match the actual behavior.

Suggested change

	if name != "user_identities" {
	// V15 was recorded with wrong name — table was never created.
	// Delete the stale record so the migration loop will reapply it.
	tracing::warn!(
	recorded_name = %name,
	"libSQL: V15 was mis-recorded; deleting stale _migrations row to reapply"
	// Only repair the specific known-bad case where V15 was recorded as
	// "document_versions" and the user_identities table was never created.
	let mut table_rows = conn
	.query(
	"SELECT name FROM sqlite_master WHERE type = 'table' AND name = 'user_identities'",
	libsql::params![],
	)
	.await
	.map_err(|e| {
	DatabaseError::Migration(format!(
	"V15 repair: failed to check for user_identities table: {e}"
	))
	})?;
	let table_exists = table_rows
	.next()
	.await
	.map_err(|e| {
	DatabaseError::Migration(format!(
	"V15 repair: failed to read user_identities existence result: {e}"
	))
	})?
	.is_some();
	if name == "document_versions" && !table_exists {
	// V15 was recorded with the wrong name and the user_identities table
	// was never created. Delete the stale record so the migration loop
	// will reapply V15 correctly.
	tracing::warn!(
	recorded_name = %name,
	"libSQL: V15 was mis-recorded as document_versions with no user_identities table; deleting stale _migrations row to reapply"

[ilblackdragon]

Agreed on tightening the condition — changed from != "user_identities" to == "document_versions" so it only repairs the specific known-bad case. Skipped the table-existence check since V15 uses CREATE TABLE IF NOT EXISTS, making re-application idempotent and harmless even if the table already exists. Fixed in 64d5752.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment in run_incremental says the repair only deletes the V15 row when it's recorded but the user_identities table doesn't exist, but the implementation currently only checks name == "document_versions" and does not verify table existence. Please update the comment to match the actual behavior (or add the existence check if that’s the intended condition).

Suggested change

	// never ran. If V15 is recorded but the table doesn't exist, delete
	// the stale record so V15 reruns with the correct SQL.
	// never ran. If V15 is recorded with the wrong name, delete the stale
	// record so V15 reruns with the correct SQL.