Renumber migrations after the WIT default shift#89
Merged
Conversation
Make `V12__wasm_wit_default_0_3_0.sql` the PostgreSQL migration that updates the default WIT version, then move the token-budget and redundant-index migrations to `V13` and `V14` so the sequence stays strictly ordered. Align the libSQL incremental migration table and its tests with the new version numbers, and update the libSQL integration test to assert the renumbered migration marker.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Migration Sequencing Update for WIT Default ShiftRenumbers and realigns libSQL incremental migrations so the WebAssembly Interface Types (WIT) default version update runs as migration V12, with subsequent migrations shifted to V13 and V14. Updates migration runner logic, non-transactional handling, SQL builders, constants, and tests to assert the new ordering. References ExecPlan: docs/plans/2026-03-09-use-wit-v3-in-extensions.md. Changes
Rationale / ContextEnsures strict, deterministic migration ordering so the WIT 0.3.0 default update runs as V12 and subsequent migrations remain ordered, aligning DB migration sequencing with the WIT 0.3.0 host contract and the ExecPlan document. Impact
WalkthroughRenumber the wasm WIT default incremental migration from 10 → 12, shift subsequent migrations to 13 and 14, rename the SQL builder function, update the runner special‑case to version 12, switch the embedded SQL include to V14, and update tests to assert the new versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as "Test / Runner"
participant Runner as "Migration Runner"
participant Builder as "v12 SQL Builder"
participant DB as "Database (_migrations & exec)"
Client->>Runner: request run_incremental(version = 12)
Runner->>DB: query `_migrations` for version 12
DB-->>Runner: return migration entry / metadata
Runner->>Builder: request non-transactional SQL for v12
Builder-->>Runner: return SQL script (non-transactional)
Runner->>DB: execute SQL (non-transactional)
DB-->>Runner: return execution result
Runner-->>Client: return success/failure
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
scope: db/postgres
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRenumbers the libSQL incremental migrations so that the WIT default-update migration is version 12, shifts subsequent migrations to versions 13 and 14, and updates the non-transactional migration helper plus tests and integration checks to match the new numbering and SQL marker strings. Class diagram for libSQL migration helper function renumberingclassDiagram
class LibsqlMigrationsModule {
+INCREMENTAL_MIGRATIONS : Vec~(i64, &str, &str)~
+SCHEMA : &str
+run_incremental(conn : libsql_Connection) Result
-append_table_rebuild_sql(sql : String, table_name : &str, columns_sql : &str) String
-v12_wasm_wit_default_migration_sql() String
}
class libsql_Connection
LibsqlMigrationsModule --> libsql_Connection : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider renaming
v12_wasm_wit_default_migration_sqlto a version-agnostic name (e.g.,wit_default_migration_sql) so future renumbering doesn’t require touching function names and all their call sites. - Some test assertion messages and strings embed the migration version number (e.g., "expected a V12 libSQL migration..."); you could reduce future churn by asserting on the name/marker and making messages less version-specific where possible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming `v12_wasm_wit_default_migration_sql` to a version-agnostic name (e.g., `wit_default_migration_sql`) so future renumbering doesn’t require touching function names and all their call sites.
- Some test assertion messages and strings embed the migration version number (e.g., "expected a V12 libSQL migration..."); you could reduce future churn by asserting on the name/marker and making messages less version-specific where possible.
## Individual Comments
### Comment 1
<location path="tests/db_integration/libsql_wit_defaults.rs" line_range="137" />
<code_context>
let mut migration_rows = conn
.query(
"SELECT name FROM _migrations WHERE version = ?1",
- libsql::params![10],
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the integration test by validating the full migration state, not just the presence of version 12
The test now checks for version 12 instead of 10, which matches the renumbered migrations. To make it a stronger regression guard around this renumbering, you could also:
- Assert that there is no `_migrations` row for version 10.
- Optionally assert that versions 13 and 14 exist with the expected names, so the test fails if libSQL and PostgreSQL migration sequences diverge again.
Suggested implementation:
```rust
// First, assert that there is no migration entry for the old version 10.
let mut migration_rows = conn
.query(
"SELECT name FROM _migrations WHERE version = ?1",
libsql::params![10],
)
.await
.expect("query migration marker for version 10");
assert!(
migration_rows
.next()
.await
.expect("read migration row for version 10")
.is_none(),
"unexpected _migrations row for version 10; migration numbering has regressed"
);
// Then, assert that the renumbered migrations (12, 13, 14) all exist.
for version in [12_i64, 13, 14] {
let mut rows = conn
.query(
"SELECT name FROM _migrations WHERE version = ?1",
libsql::params![version],
)
.await
.expect("query migration marker");
let row = rows
.next()
.await
.expect("read migration row for expected version");
assert!(
row.is_some(),
"expected _migrations row for version {}; migration sequences may have diverged",
version
);
}
// Preserve the original query for version 12 in case later assertions depend on it.
let mut migration_rows = conn
.query(
"SELECT name FROM _migrations WHERE version = ?1",
libsql::params![12],
)
.await
.expect("query migration marker");
```
If you want to assert specific migration names for versions 12, 13, and 14 (not just their presence), you can extend the loop body to extract the `name` column from `row` using whatever accessor your `libsql` row type provides (e.g., `row.unwrap().get::<_, String>(0)?`) and compare it to the expected migration filenames. That part depends on the exact APIs and naming conventions used elsewhere in your test suite.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ebcf8deff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/libsql_migrations.rs (1)
68-157:⚠️ Potential issue | 🟡 MinorDocument the intentional libSQL migration version gap (9 → 12) in the code.
V10 (wasm_versioning) and V11 (conversation_unique_indexes) changes are embedded directly in
libsql_schema.sql, whereas PostgreSQL applies them as incremental migrations. TheINCREMENTAL_MIGRATIONSarray correctly skips these versions because libSQL's bootstrap schema already includes those schema updates. Add a comment above the array explaining this backend-specific design to prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/libsql_migrations.rs` around lines 68 - 157, The INCREMENTAL_MIGRATIONS array currently jumps from version 9 to 12 which looks like a bug; add a short explanatory comment immediately above the INCREMENTAL_MIGRATIONS definition stating that the v10 (wasm_versioning) and v11 (conversation_unique_indexes) migrations are intentionally omitted for libSQL because those schema changes are already baked into the libSQL bootstrap schema (libsql_schema.sql) rather than applied incrementally, and that this gap is backend-specific to prevent future confusion when editing INCREMENTAL_MIGRATIONS or adding new versions; reference the symbols INCREMENTAL_MIGRATIONS, the numeric versions 9→12, and the migration names (wasm_versioning, conversation_unique_indexes) in the comment so maintainers can quickly understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/db/libsql_migrations.rs`:
- Around line 68-157: The INCREMENTAL_MIGRATIONS array currently jumps from
version 9 to 12 which looks like a bug; add a short explanatory comment
immediately above the INCREMENTAL_MIGRATIONS definition stating that the v10
(wasm_versioning) and v11 (conversation_unique_indexes) migrations are
intentionally omitted for libSQL because those schema changes are already baked
into the libSQL bootstrap schema (libsql_schema.sql) rather than applied
incrementally, and that this gap is backend-specific to prevent future confusion
when editing INCREMENTAL_MIGRATIONS or adding new versions; reference the
symbols INCREMENTAL_MIGRATIONS, the numeric versions 9→12, and the migration
names (wasm_versioning, conversation_unique_indexes) in the comment so
maintainers can quickly understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8875d58-8b43-49ad-8b29-4a68cbc7d85b
📒 Files selected for processing (5)
migrations/V12__wasm_wit_default_0_3_0.sqlmigrations/V13__job_token_budget.sqlmigrations/V14__drop_redundant_wasm_tools_name_index.sqlsrc/db/libsql_migrations.rstests/db_integration/libsql_wit_defaults.rs
Expand the libSQL WIT-default integration test so it asserts the old `V10` migration marker is absent and the renumbered `V12`, `V13`, and `V14` markers all exist with the expected names. Keep the renumbering fix covered by a stronger regression check so a future divergence between PostgreSQL and libSQL migration sequences fails loudly.
github-actions
Bot
added
size: M
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/db_integration/libsql_wit_defaults.rs (1)
54-175: 🧹 Nitpick | 🔵 TrivialFunction length exceeds recommended threshold.
Static analysis flags this function at 115 lines against a 70-line threshold. Extract helper functions to reduce cognitive load:
♻️ Suggested helper extractions
async fn assert_migration_absent(conn: &libsql::Connection, version: i64) { let mut rows = conn .query( "SELECT name FROM _migrations WHERE version = ?1", libsql::params![version], ) .await .expect("query migration marker"); assert!( rows.next().await.expect("read migration row").is_none(), "unexpected _migrations row for version {version}; migration numbering regressed" ); } async fn assert_migration_present(conn: &libsql::Connection, version: i64, expected_name: &str) { let mut rows = conn .query( "SELECT name FROM _migrations WHERE version = ?1", libsql::params![version], ) .await .expect("query migration marker"); let row = rows .next() .await .expect("read migration row") .unwrap_or_else(|| { panic!("expected _migrations row for version {version}; migration sequences diverged") }); let name: String = row.get(0).expect("migration name"); assert_eq!(name, expected_name); }Then the test body becomes:
assert_migration_absent(&conn, 10).await; for (version, expected_name) in [ (12_i64, "wasm_wit_default_0_3_0"), (13_i64, "job_token_budget"), (14_i64, "drop_redundant_wasm_tools_name_index"), ] { assert_migration_present(&conn, version, expected_name).await; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/db_integration/libsql_wit_defaults.rs` around lines 54 - 175, The test function libsql_run_migrations_upgrades_legacy_wasm_wit_defaults is too long; extract two small async helpers (e.g., assert_migration_absent(conn: &libsql::Connection, version: i64) and assert_migration_present(conn: &libsql::Connection, version: i64, expected_name: &str)) that encapsulate the repeated _migrations query/validation logic currently using conn.query(...).await, then call assert_migration_absent(&conn, 10).await and replace the loop body with calls to assert_migration_present(&conn, version, expected_name).await for each (version, expected_name) tuple to shorten and clarify the main test function while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/db_integration/libsql_wit_defaults.rs`:
- Around line 54-175: The test function
libsql_run_migrations_upgrades_legacy_wasm_wit_defaults is too long; extract two
small async helpers (e.g., assert_migration_absent(conn: &libsql::Connection,
version: i64) and assert_migration_present(conn: &libsql::Connection, version:
i64, expected_name: &str)) that encapsulate the repeated _migrations
query/validation logic currently using conn.query(...).await, then call
assert_migration_absent(&conn, 10).await and replace the loop body with calls to
assert_migration_present(&conn, version, expected_name).await for each (version,
expected_name) tuple to shorten and clarify the main test function while
preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 353482ab-cb5c-4e8f-823f-ed97ab3de285
📒 Files selected for processing (1)
tests/db_integration/libsql_wit_defaults.rs
Document why `INCREMENTAL_MIGRATIONS` jumps from `9` to `12` in the libSQL backend. Explain that `wasm_versioning` and `conversation_unique_indexes` are already baked into `libsql_schema.sql`, so maintainers do not try to add backend-specific placeholder entries when extending the migration sequence.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/libsql_migrations.rs (1)
24-62: 🧹 Nitpick | 🔵 TrivialRename these constants to reflect V12 usage.
V10_WASM_TOOLS_COLUMNS,V10_WASM_TOOLS_COPY_COLUMNS,V10_WASM_TOOLS_POST_REBUILD_SQL,V10_WASM_CHANNELS_COLUMNS, andV10_WASM_CHANNELS_COPY_COLUMNSare now used exclusively byv12_wasm_wit_default_migration_sql(), yet retain theV10_prefix. This creates a naming inconsistency that could confuse maintainers.Rename these constants to
V12_*or drop the version prefix entirely (e.g.,WASM_WIT_DEFAULT_TOOLS_COLUMNS) to match their actual usage context.♻️ Proposed rename
-const V10_WASM_TOOLS_COLUMNS: &str = r#" id TEXT PRIMARY KEY, +const V12_WASM_TOOLS_COLUMNS: &str = r#" id TEXT PRIMARY KEY, user_id TEXT NOT NULL, ... UNIQUE (user_id, name, version)"#; -const V10_WASM_TOOLS_COPY_COLUMNS: &str = r#"id, user_id, name, version, wit_version, description, wasm_binary, binary_hash, +const V12_WASM_TOOLS_COPY_COLUMNS: &str = r#"id, user_id, name, version, wit_version, description, wasm_binary, binary_hash, parameters_schema, source_url, trust_level, status, created_at, updated_at"#; -const V10_WASM_TOOLS_POST_REBUILD_SQL: &str = r#"CREATE INDEX IF NOT EXISTS idx_wasm_tools_user ON wasm_tools(user_id); +const V12_WASM_TOOLS_POST_REBUILD_SQL: &str = r#"CREATE INDEX IF NOT EXISTS idx_wasm_tools_user ON wasm_tools(user_id); CREATE INDEX IF NOT EXISTS idx_wasm_tools_status ON wasm_tools(status); CREATE INDEX IF NOT EXISTS idx_wasm_tools_trust ON wasm_tools(trust_level);"#; -const V10_WASM_CHANNELS_COLUMNS: &str = r#" id TEXT PRIMARY KEY, +const V12_WASM_CHANNELS_COLUMNS: &str = r#" id TEXT PRIMARY KEY, user_id TEXT NOT NULL, ... UNIQUE (user_id, name)"#; -const V10_WASM_CHANNELS_COPY_COLUMNS: &str = r#"id, user_id, name, version, wit_version, description, wasm_binary, binary_hash, +const V12_WASM_CHANNELS_COPY_COLUMNS: &str = r#"id, user_id, name, version, wit_version, description, wasm_binary, binary_hash, capabilities_json, status, created_at, updated_at"#;Then update the references in
v12_wasm_wit_default_migration_sql()at lines 317–329 accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/libsql_migrations.rs` around lines 24 - 62, Constants with a V10_ prefix (V10_WASM_TOOLS_COLUMNS, V10_WASM_TOOLS_COPY_COLUMNS, V10_WASM_TOOLS_POST_REBUILD_SQL, V10_WASM_CHANNELS_COLUMNS, V10_WASM_CHANNELS_COPY_COLUMNS) are actually used by v12_wasm_wit_default_migration_sql() and should be renamed to reflect V12 usage (or a neutral name) to avoid confusion; rename each constant (e.g., V12_WASM_TOOLS_COLUMNS, V12_WASM_TOOLS_COPY_COLUMNS, V12_WASM_TOOLS_POST_REBUILD_SQL, V12_WASM_CHANNELS_COLUMNS, V12_WASM_CHANNELS_COPY_COLUMNS or drop the version prefix) and update every reference inside v12_wasm_wit_default_migration_sql() to the new names so the code compiles and the names match their usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/db/libsql_migrations.rs`:
- Around line 24-62: Constants with a V10_ prefix (V10_WASM_TOOLS_COLUMNS,
V10_WASM_TOOLS_COPY_COLUMNS, V10_WASM_TOOLS_POST_REBUILD_SQL,
V10_WASM_CHANNELS_COLUMNS, V10_WASM_CHANNELS_COPY_COLUMNS) are actually used by
v12_wasm_wit_default_migration_sql() and should be renamed to reflect V12 usage
(or a neutral name) to avoid confusion; rename each constant (e.g.,
V12_WASM_TOOLS_COLUMNS, V12_WASM_TOOLS_COPY_COLUMNS,
V12_WASM_TOOLS_POST_REBUILD_SQL, V12_WASM_CHANNELS_COLUMNS,
V12_WASM_CHANNELS_COPY_COLUMNS or drop the version prefix) and update every
reference inside v12_wasm_wit_default_migration_sql() to the new names so the
code compiles and the names match their usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb6656c8-29dd-46b4-ab28-ac3c532d9fb9
📒 Files selected for processing (1)
src/db/libsql_migrations.rs
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Large Methodtests/db_integration/libsql_wit_defaults.rs: libsql_run_migrations_upgrades_legacy_wasm_wit_defaults What lead to degradation?libsql_run_migrations_upgrades_legacy_wasm_wit_defaults increases from 90 to 115 lines of code, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
This comment was marked as resolved.
This comment was marked as resolved.
Rename the libSQL rebuild SQL constants that feed v12_wasm_wit_default_migration_sql so their names match the migration version that still uses them. This removes stale V10-prefixed identifiers that were left behind after migration renumbering and makes the v12 migration wiring easier to follow.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract the repeated insert and assertion steps from `libsql_run_migrations_upgrades_legacy_wasm_wit_defaults` into free async helpers in the same integration test file. This keeps the legacy schema fixture unchanged while shrinking the test body down to setup, migration execution, and the expected helper calls.
Owner
Author
|
@coderabbitai Have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6442
The test is now a proper regression guard for the full renumbering. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make
V12__wasm_wit_default_0_3_0.sqlthe PostgreSQL migration that updates the default WIT version, then move the token-budget and redundant-index migrations toV13andV14so the sequence stays strictly ordered.Align the libSQL incremental migration table and its tests with the new version numbers, and update the libSQL integration test to assert the renumbered migration marker.
Summary by Sourcery
Renumber libSQL incremental migrations to align the WIT default version shift and keep migration sequencing consistent across backends.
Enhancements:
Tests: