feat(P3): Epic 3.6 — MigrationManager extraction from SkillStore#23
Merged
Deepfreezechill merged 2 commits intomainfrom Apr 2, 2026
Merged
feat(P3): Epic 3.6 — MigrationManager extraction from SkillStore#23Deepfreezechill merged 2 commits intomainfrom
Deepfreezechill merged 2 commits intomainfrom
Conversation
- Extract DDL and schema initialization logic into MigrationManager - Support standalone (db_path) and embedded (conn + lock) modes - Add schema versioning with PRAGMA user_version - Add migration paths and ensure_current_schema() for app init - Remove _DDL constant and _init_db method from SkillStore - Add facade methods to SkillStore for MigrationManager API - Update __init__.py to export MigrationManager - Comprehensive tests for schema creation, versioning, migration - Tests verify SkillStore integration and facade methods - Follows same extraction pattern as Epic 3.2-3.5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔒 Phase Gate Enforcement — 🚫 FAIL
Verdict: FAIL
|
…jection guard ## 🔥 CRITICAL FIXES (ALL 6 /8EYES FINDINGS RESOLVED) ### Finding 1: DDL consolidation (CRITICAL) - ✅ MigrationManager is now the SINGLE source of truth for all DDL - ✅ Removed duplicate _DDL constants from SkillRepository, AnalysisStore, TagSearch - ✅ All modules delegate to MigrationManager.ensure_current_schema() in standalone mode - ✅ Fixed schema divergence: skill_judgments now has UNIQUE(analysis_id, skill_id) everywhere - ✅ Architecture invariant: CREATE TABLE strings only exist in MigrationManager ### Finding 2: Atomic migration (MAJOR) - ✅ migrate_to_version() now wraps DDL + version bump in single transaction - ✅ Uses individual execute() statements instead of executescript() for transaction control - ✅ Proper rollback on failure prevents inconsistent state (version != schema) ### Finding 3: PRAGMA f-string injection (MAJOR) - ✅ Added explicit isinstance(version, int) type check before f-string - ✅ Added bounds checking (0 ≤ version ≤ 99) to prevent bypass - ✅ Made set_schema_version() private (_set_schema_version) with deprecation warning ### Finding 4: Legacy v0→1 doesn't actually upgrade (MAJOR) - ✅ Documented that 0→1 migration is 'fresh install' (no legacy DBs exist) - ✅ Added TODO for future v1→v2 migrations to use ALTER TABLE - ✅ Fixed atomic execution to ensure real schema creation ### Finding 5: Version bypass (MINOR) - ✅ Made set_schema_version() private to prevent external bypass - ✅ Public API now only uses ensure_current_schema() - ✅ Added deprecation warning for existing facade methods ### Finding 6: Missing tests (MINOR) - ✅ test_set_schema_version_rejects_non_int — injection prevention - ✅ test_ddl_single_source_of_truth — no CREATE TABLE in other modules - ✅ test_standalone_modules_use_migration_manager — delegation verification - ✅ test_migration_atomic_on_failure — rollback on DDL failure ## 📊 TEST RESULTS - Migration manager tests: 33/33 ✅ - Full test suite: 1309 passed, 31 skipped ✅ - Fixed schema inconsistency: UNIQUE constraint now properly enforced - Updated analysis_store tests for new constraint behavior ## 🏗️ ARCHITECTURE IMPROVEMENT Single source of truth established. Schema divergence impossible. Future-proof foundation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
VonkraLLC
approved these changes
Apr 2, 2026
VonkraLLC
left a comment
VonkraLLC
left a comment
There was a problem hiding this comment.
LGTM — /8eyes R1 + R2: DDL consolidated to MigrationManager, atomic migration, injection guard, version bypass fixed. R2 partials are cosmetic (wrapper methods, not duplicate DDL).
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
2 participants
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.
Epic 3.6: MigrationManager Extraction
What
Extracts DDL, schema creation, versioning, and migration logic from the monolithic SkillStore into a focused MigrationManager class.
Changes
Results
Closes #22