feat: implement Alembic migrations - #68
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR replaces embedded migrations with Alembic. It adds the Alembic runtime configuration, creates the v8 SQLite schema through an initial revision, and updates ChangesAlembic migration integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationManager
participant Alembic
participant AlembicEnv
participant SQLiteDatabase
MigrationManager->>Alembic: run upgrade to head
Alembic->>AlembicEnv: load migration environment
AlembicEnv->>SQLiteDatabase: apply baseline migration
MigrationManager->>SQLiteDatabase: query alembic_version
SQLiteDatabase-->>MigrationManager: return version or None
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryReplaces the custom eight-step SQLite migration manager with Alembic, adds a baseline v8 schema revision, and resolves the migration database dynamically.
Confidence Score: 2/5This PR is not safe to merge until existing databases are upgraded correctly, migrations target the requested database, and the readiness check supports Alembic revision identifiers. Existing installations can be stamped at the Alembic baseline without receiving required columns, injected connection managers migrate a different file, and every readiness request treats the new revision value as an incompatible integer. Files Needing Attention: alembic/versions/a38d67fcd99e_init_v8_schema.py, alembic/env.py, shared/migrations.py
|
| Filename | Overview |
|---|---|
| shared/migrations.py | Replaces incremental migrations with Alembic but breaks legacy upgrade handling, injected database targeting, and the readiness consumer’s numeric version contract. |
| alembic/env.py | Configures online and offline Alembic execution but resolves the database independently from MigrationManager’s connection manager. |
| alembic/versions/a38d67fcd99e_init_v8_schema.py | Defines the final v8 schema for fresh databases but cannot bring existing pre-v8 tables to that schema. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Server startup] --> B[MigrationManager.migrate]
B --> C[get_current_version via AsyncConnectionManager]
B --> D[Alembic upgrade in worker thread]
D --> E[env.py resolves memory.db]
E --> F[Apply baseline v8 revision]
F --> G[Create or stamp alembic_version]
B --> H[Read resulting revision]
Prompt To Fix All With AI
### Issue 1
alembic/versions/a38d67fcd99e_init_v8_schema.py:21-30
**Legacy schemas remain incomplete**
If an existing database was created by a pre-v8 custom migration, Alembic runs this baseline but `CREATE TABLE IF NOT EXISTS` leaves existing table definitions unchanged. Alembic then records the revision while required columns such as `core_memory.memory_kind` and `rag_chunks.bin_embedding` remain absent, causing subsequent writes and searches to fail with SQLite schema errors.
### Issue 2
alembic/env.py:14-17
**Migration targets the wrong database**
When `MigrationManager` receives an `AsyncConnectionManager` with a custom `base_dir`, version checks use that directory while Alembic independently resolves the database from `MCP_MEMORY_DATA_DIR`. The upgrade therefore modifies another database and leaves the requested database unmigrated, causing missing-table or missing-column failures for custom-directory consumers.
### Issue 3
shared/migrations.py:32-38
**Revision type breaks readiness**
`get_current_version()` now returns an Alembic revision string or `None`, but the readiness endpoint still compares the result with integer `2`. That comparison raises `TypeError`, which the endpoint converts into `ready=false` and `migration_version=0`, so health-check-driven deployments never become ready after a successful migration.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat: implement Alembic migrations for m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def upgrade() -> None: | ||
| # 1. Core tables | ||
| op.execute(""" | ||
| CREATE TABLE IF NOT EXISTS core_memory ( | ||
| entry_id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| user_id TEXT NOT NULL, key TEXT NOT NULL, value TEXT NOT NULL, | ||
| importance REAL DEFAULT 0.5, is_conflict INTEGER DEFAULT 0, | ||
| conflict_group_id TEXT, created_at REAL NOT NULL, updated_at REAL NOT NULL, | ||
| memory_kind TEXT, expires_at REAL, source TEXT DEFAULT 'manual', metadata TEXT | ||
| ) |
There was a problem hiding this comment.
If an existing database was created by a pre-v8 custom migration, Alembic runs this baseline but CREATE TABLE IF NOT EXISTS leaves existing table definitions unchanged. Alembic then records the revision while required columns such as core_memory.memory_kind and rag_chunks.bin_embedding remain absent, causing subsequent writes and searches to fail with SQLite schema errors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: alembic/versions/a38d67fcd99e_init_v8_schema.py
Line: 21-30
Comment:
**Legacy schemas remain incomplete**
If an existing database was created by a pre-v8 custom migration, Alembic runs this baseline but `CREATE TABLE IF NOT EXISTS` leaves existing table definitions unchanged. Alembic then records the revision while required columns such as `core_memory.memory_kind` and `rag_chunks.bin_embedding` remain absent, causing subsequent writes and searches to fail with SQLite schema errors.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| def get_url(): | ||
| data_dir = os.environ.get("MCP_MEMORY_DATA_DIR", str(Path.home() / ".mcp-ariel-memory")) | ||
| db_path = Path(data_dir) / "memory.db" | ||
| return f"sqlite:///{db_path}" |
There was a problem hiding this comment.
When MigrationManager receives an AsyncConnectionManager with a custom base_dir, version checks use that directory while Alembic independently resolves the database from MCP_MEMORY_DATA_DIR. The upgrade therefore modifies another database and leaves the requested database unmigrated, causing missing-table or missing-column failures for custom-directory consumers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: alembic/env.py
Line: 14-17
Comment:
**Migration targets the wrong database**
When `MigrationManager` receives an `AsyncConnectionManager` with a custom `base_dir`, version checks use that directory while Alembic independently resolves the database from `MCP_MEMORY_DATA_DIR`. The upgrade therefore modifies another database and leaves the requested database unmigrated, causing missing-table or missing-column failures for custom-directory consumers.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| async def get_current_version(self) -> str | None: | ||
| conn = await self._cm.get(DB_NAME) | ||
| try: | ||
| row = await (await conn.execute("SELECT MAX(version) as v FROM migration_log")).fetchone() | ||
| return row["v"] if row and row["v"] else 0 | ||
| except sqlite3.OperationalError: | ||
| return 0 | ||
| row = await (await conn.execute("SELECT version_num FROM alembic_version")).fetchone() | ||
| return row["version_num"] if row else None | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
get_current_version() now returns an Alembic revision string or None, but the readiness endpoint still compares the result with integer 2. That comparison raises TypeError, which the endpoint converts into ready=false and migration_version=0, so health-check-driven deployments never become ready after a successful migration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: shared/migrations.py
Line: 32-38
Comment:
**Revision type breaks readiness**
`get_current_version()` now returns an Alembic revision string or `None`, but the readiness endpoint still compares the result with integer `2`. That comparison raises `TypeError`, which the endpoint converts into `ready=false` and `migration_version=0`, so health-check-driven deployments never become ready after a successful migration.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.1 participant
Replaced custom migration manager with Alembic. Includes baseline v8 schema migration and dynamic DB path resolution.
Summary by CodeRabbit
New Features
Bug Fixes