test: cover model-engine migration adoption

[ravneet-madaan-scale]

Adds automated regression coverage to #852 for the database-adoption behavior that the SGP model-engine recovery depends on.

The tests cover:

• adopting a pre-existing hosted_model_inference schema without replaying initial.sql;
• initializing an empty database through the initial revision;
• skipping existing columns during every add-column upgrade;
• adding only missing columns;
• skipping absent columns during downgrade; and
• dropping existing columns during downgrade.

This deliberately targets the open fix branch so it strengthens #852 without duplicating or overwriting the author's implementation.

Validation:

• 30 focused pytest cases pass;
• Black 24.8.0 passes;
• isort 5.13.2 passes;
• Ruff 0.6.8 passes.

Real PostgreSQL validation also passes for both an empty database and a simulated
legacy database with an existing, unstamped hosted_model_inference schema. Each
case successfully completed upgrade, repeat upgrade, downgrade, and re-upgrade.

Greptile Summary

Adds a new test file that provides regression coverage for the idempotent database-migration adoption behavior introduced in #852. Tests load each migration via importlib, replace sa.inspect and op with mocks via monkeypatch, and assert upgrade/downgrade outcomes without hitting a real database.

• 30 parametrised cases cover all seven ADD_COLUMN migrations (skip-if-exists and add-if-missing for upgrade; skip-if-absent and drop-if-present for downgrade) plus two cases for the initial migration (adopt existing schema vs. run initial.sql on an empty database).
• Migration modules are freshly loaded per test via spec_from_file_location + exec_module, isolating module-level state while sharing the real sqlalchemy module object (safe because monkeypatch is function-scoped and restores teardown).

Confidence Score: 4/5

Safe to merge — only adds tests, touches no production code.

The test logic correctly exercises all six idempotency scenarios across every migration in the chain. The single gap is that the get_columns mock is checked only for its return value, not for whether it was called with the correct schema; a migration that dropped the schema keyword from the inspector call would still make all tests pass. This is a test-coverage quality concern rather than a correctness defect in production behaviour.

No production files are changed. The only file worth a second look is test_database_migration_adoption.py — specifically the get_columns assertion gaps noted in the inline comment.

Important Files Changed

Filename	Overview
model-engine/tests/test_database_migration_adoption.py	New test file covering idempotent migration adoption: adopt existing schema vs. fresh initialisation, skip/add missing columns on upgrade, skip/drop existing columns on downgrade. Logic is sound for current migrations; the get_columns mock doesn't assert schema arguments, leaving one narrow gap in coverage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[load_migration - importlib] --> B{Test variant}
    B --> C[test_initial_migration_adopts_existing_schema]
    B --> D[test_initial_migration_initializes_empty_database]
    B --> E[test_upgrade_skips_columns_that_already_exist]
    B --> F[test_upgrade_adds_only_missing_columns]
    B --> G[test_downgrade_skips_columns_that_are_already_absent]
    B --> H[test_downgrade_drops_only_existing_columns]
    C --> C1["inspector.has_table True: open and op.execute NOT called"]
    D --> D1["inspector.has_table False: open called, op.execute called with SQL"]
    E --> E1["get_columns all columns: op.add_column NOT called"]
    F --> F1["get_columns empty: op.add_column called with correct table/column/schema"]
    G --> G1["get_columns empty: op.drop_column NOT called"]
    H --> H1["get_columns all columns: op.drop_column called with correct table/column/schema"]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[load_migration - importlib] --> B{Test variant}
    B --> C[test_initial_migration_adopts_existing_schema]
    B --> D[test_initial_migration_initializes_empty_database]
    B --> E[test_upgrade_skips_columns_that_already_exist]
    B --> F[test_upgrade_adds_only_missing_columns]
    B --> G[test_downgrade_skips_columns_that_are_already_absent]
    B --> H[test_downgrade_drops_only_existing_columns]
    C --> C1["inspector.has_table True: open and op.execute NOT called"]
    D --> D1["inspector.has_table False: open called, op.execute called with SQL"]
    E --> E1["get_columns all columns: op.add_column NOT called"]
    F --> F1["get_columns empty: op.add_column called with correct table/column/schema"]
    G --> G1["get_columns empty: op.drop_column NOT called"]
    H --> H1["get_columns all columns: op.drop_column called with correct table/column/schema"]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
model-engine/tests/test_database_migration_adoption.py:88-105
**Schema argument not validated in `get_columns` mock**

The `inspector.get_columns` mock always returns a fixed list regardless of the arguments it receives. None of the four test variants assert that `get_columns` was called with `schema="hosted_model_inference"`. A migration that omits the `schema` keyword from its internal `get_columns` call would silently produce the same mock return value and the tests would still pass, masking the missing schema propagation. Adding `inspector.get_columns.assert_any_call(table, schema="hosted_model_inference")` (or equivalent per-call assertions) in the upgrade/downgrade tests would close this gap — the same applies to `test_downgrade_skips_columns_that_are_already_absent` and `test_downgrade_drops_only_existing_columns`.

_{Reviews (1): Last reviewed commit: "test: cover model-engine migration adopt..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[greptile-apps]

Schema argument not validated in get_columns mock

The inspector.get_columns mock always returns a fixed list regardless of the arguments it receives. None of the four test variants assert that get_columns was called with schema="hosted_model_inference". A migration that omits the schema keyword from its internal get_columns call would silently produce the same mock return value and the tests would still pass, masking the missing schema propagation. Adding inspector.get_columns.assert_any_call(table, schema="hosted_model_inference") (or equivalent per-call assertions) in the upgrade/downgrade tests would close this gap — the same applies to test_downgrade_skips_columns_that_are_already_absent and test_downgrade_drops_only_existing_columns.

Prompt To Fix With AI

This is a comment left during a code review.
Path: model-engine/tests/test_database_migration_adoption.py
Line: 88-105

Comment:
**Schema argument not validated in `get_columns` mock**

The `inspector.get_columns` mock always returns a fixed list regardless of the arguments it receives. None of the four test variants assert that `get_columns` was called with `schema="hosted_model_inference"`. A migration that omits the `schema` keyword from its internal `get_columns` call would silently produce the same mock return value and the tests would still pass, masking the missing schema propagation. Adding `inspector.get_columns.assert_any_call(table, schema="hosted_model_inference")` (or equivalent per-call assertions) in the upgrade/downgrade tests would close this gap — the same applies to `test_downgrade_skips_columns_that_are_already_absent` and `test_downgrade_drops_only_existing_columns`.

How can I resolve this? If you propose a fix, please make it concise.

[ravneet-madaan-scale]

Closing this follow-up: it adds coverage for the proposed migration fix, but we have not yet confirmed that migration behavior caused the observed SGP deployment failure.