migration test fixes - #4093
Merged
Merged
Conversation
Contributor
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates migration-test script to use stable transports/v* tag selection, expands faker seeds (new budget and ownership), adds MCP per-user-header generators/wiring, extends schema normalization and multi-budget cleanup for Postgres/SQLite, augments dynamic INSERTs with optional JSON columns, skips a snapshot table, and introduces dropColumnSQL plus safer SQLite connection pinning in Go migrations. ChangesMigration Test Infrastructure & Go migration compatibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
akshaydeo
marked this pull request as ready for review
June 5, 2026 17:30
Contributor
Confidence Score: 4/5Safe to merge with one outstanding test-coverage gap that will surface as a test failure once the next stable release enters the tested version window. The migrations.go changes are well-structured and correct. The test script gap —
Important Files Changed
Reviews (5): Last reviewed commit: "migration test fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 492-499: The test harness seeds a customer with budget
'budget-migration-test-3' but does not assert customer-owned budget migration;
update verify_budget_migration_postgres() to include an assertion that the
governance_customers table's budget_id for 'customer-migration-test-1' has been
migrated to the new ownership model (or that the corresponding
governance_budgets.<owner>_id reflects the customer as owner), and add a
dedicated check that the customer-budget path (customer-migration-test-1 ->
budget-migration-test-3) is present and correct after migration so a broken
customer-budget backfill will fail the test.
- Around line 3929-3933: Remove governance_model_configs from the skip_tables
list and instead special-case it in the validation logic: keep full value
comparisons for seeded rows matching the 'model-config-migration-test-*' pattern
but relax (ignore) the row-count delta check for governance_model_configs.
Concretely, update the skip_tables variable to no longer include
governance_model_configs, and modify the post-migration validation routine (the
code that iterates skip_tables and compares before/after rows) to detect table
== "governance_model_configs" and, for that table, skip only the row-count
assertion while still performing equality checks for rows whose key/name matches
'model-config-migration-test-*'.
- Around line 27-28: Uncomment and re-enable fetching tags in the CI script so
get_previous_versions() can discover local transports/v* tags: restore the
previously commented "git fetch --tags" line in run-migration-tests.sh so that
the get_previous_versions() function (which reads local transports/v* tags) runs
against a repo with tags present, ensuring the migration matrix is generated
from real tags rather than falling back to hard-coded prereleases.
In `@framework/configstore/migrations.go`:
- Around line 9730-9731: The sqlite foreign-keys workaround currently calls
sqlDB.SetMaxOpenns(1) then unconditionally defers sqlDB.SetMaxOpenConns(0);
instead capture the existing max open conns before changing it (e.g. prev :=
sqlDB.Stats().MaxOpenConnections or use sqlDB.SetMaxOpenConns to both read and
set if available) and defer restoring that captured value (defer
sqlDB.SetMaxOpenConns(prev)). Update every place that does this workaround
(including the migrationAddCustomerBudgetsToBudgetsTable block) to store the
prior value in a local variable and restore that variable in the deferred call
so you don’t hardcode 0 and don’t permanently change the caller’s configured
cap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a6d1a51-70fe-439b-88a6-7f56c9096731
📒 Files selected for processing (2)
.github/workflows/scripts/run-migration-tests.shframework/configstore/migrations.go
akshaydeo
force-pushed
the
06-05-migration_test_fixes
branch
from
June 5, 2026 17:47
14f1815 to
81c5504
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 947-951: The current seed updates in run-migration-tests.sh use
default empty payloads for JSON columns, so change the INSERT/UPDATE statements
that reference governance_model_pricing.additional_attributes (and similarly
tls_config_json, per_user_header_keys_json, headers_json at the other locations)
to write non-default representative JSON values instead of '{}' or '[]' or NULL:
locate the column_exists_postgres checks and the echo lines that append SQL to
"$output_file" and replace the payload strings with meaningful sample JSON
(e.g., include a key/value pair and nested structure) for at least one row per
seed so the migration tests verify preservation of non-empty JSON; keep using
the same symbols column_exists_postgres and "$output_file" to find and modify
each affected block (also update the other blocks around lines mentioned in the
comment: 1989-1993, 2904-2914, 3167-3177, 3486-3490, 3519-3522).
- Around line 136-141: The get_previous_versions function can abort under set
-euo pipefail when there are no stable tags because grep returns non-zero;
change the pipeline so failures produce an empty result instead of exiting (for
example, capture tags with tags="$(git tag -l "transports/v*" || true)" and then
pipe that through grep -v -- "-" | sort -V | tail -n "$count" | sed
's|transports/||'), or alternatively append || true to the git tag or grep
command to ensure the function returns an empty string rather than causing the
script to exit; update references in get_previous_versions accordingly.
In `@framework/configstore/migrations.go`:
- Around line 3965-3980: The migration inserts a global wildcard model row for a
provider but if a pre-existing model_name='*' row exists it skips insertion and
then proceeds to NULL out config_providers.budget_id/rate_limit_id,
unintentionally dropping governance; update the logic around the
tx.Table((tables.TableModelConfig{}).TableName()).Create(...) call to perform an
upsert/merge (or use FirstOrCreate/Assign semantics) keyed on
model_name=tables.ModelConfigAllModels and provider=providerName so that an
existing wildcard row is preserved/updated instead of skipped, then only clear
config_providers.budget_id and rate_limit_id when you have explicitly
moved/merged the governance into the wildcard row (use the same providerName and
p.BudgetID/p.RateLimitID values to decide whether to transfer or skip clearing).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 696c42af-da07-4f4b-81ac-f7aa8e28ae52
📒 Files selected for processing (2)
.github/workflows/scripts/run-migration-tests.shframework/configstore/migrations.go
akshaydeo
force-pushed
the
06-05-migration_test_fixes
branch
from
June 5, 2026 18:21
81c5504 to
657ad7b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/scripts/run-migration-tests.sh (3)
4348-4356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the customer-budget backfill check fail the suite.
This branch only warns when
budget-migration-test-3is not re-owned bycustomer-migration-test-1, so the regression this PR is targeting can still pass CI. If older schemas really need to be tolerated, gate that on an explicit legacy-schema check; otherwise setfailed=1here.Minimal change
if [ "$customer_budget_count" = "1" ]; then log_info " Customer budget migration: budget-migration-test-3 → customer-migration-test-1 ✓" else - log_warn " Customer budget migration: budget-migration-test-3 customer_id not set (count=$customer_budget_count) — may be expected if old version didn't have budget_id on governance_customers" + log_error " Customer budget migration: budget-migration-test-3 customer_id not set (count=$customer_budget_count)" + failed=1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/scripts/run-migration-tests.sh around lines 4348 - 4356, The customer-budget backfill check currently only logs a warning when customer_budget_count is not 1; change it to mark the test suite as failed by setting failed=1 when the query for governance_budgets (using run_postgres_scalar) does not return 1, i.e. inside the else branch after computing customer_budget_count; reference the existing variables customer_budget_count and failed and replace or augment the log_warn call (or add conditional gating if you prefer to preserve legacy behavior behind an explicit legacy-schema check) so the test run fails on this regression instead of merely warning.
3940-3944:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't skip
governance_model_configswholesale.The row-count growth is expected, but this also disables value checks for the seeded
model-config-migration-test-*rows. A migration that mutates or drops existing model-config data will now pass unnoticed. Relax only the row-count assertion for this table and keep comparing the pre-migration seeded rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/scripts/run-migration-tests.sh around lines 3940 - 3944, The script currently excludes governance_model_configs entirely via the skip_tables variable which prevents any seeded row-value checks; instead remove governance_model_configs from skip_tables and implement a targeted relaxation: continue to run the seeded-row content assertions for governance_model_configs (matching the seeded key pattern "model-config-migration-test-*") but skip only the total-row-count assertion for that table. Adjust the test logic that performs row-count vs. value comparisons (the code paths using skip_tables and the row-count check) to special-case governance_model_configs so count assertions are bypassed while seeded-row equality checks still run.
955-959:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
additional_attributesstill isn't covered by a real regression check.These writes use the default
'{}', andgovernance_model_pricingis skipped later in PostgreSQL snapshot comparison while SQLite only checks table presence. A migration that clears or rewrites this field can still pass. Seed a non-default payload and compare the preserved seeded rows explicitly if you want this v1.5.6 coverage to catch regressions.Also applies to: 1997-2001
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/scripts/run-migration-tests.sh around lines 955 - 959, The update currently seeds governance_model_pricing.additional_attributes with the default '{}' which won't detect regressions; change the seeding to write a non-default JSON payload (e.g., '{"seeded_test":"v1.5.6"}') when the script appends the UPDATE statements for governance_model_pricing (the block that checks column_exists_postgres "governance_model_pricing" "additional_attributes" and writes to "$output_file"), and add an explicit post-migration comparison/assertion that checks those specific seeded rows (by id) are preserved and match the non-default payload in the PostgreSQL snapshot step; apply the same change to the analogous block(s) referenced around the other occurrences with the same pattern so the regression check actually detects clears/rewrites of additional_attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 3993-4009: The merge currently only backfills missing governance
slots and can silently orphan a provider-owned governance row if a wildcard
(scope=global, model_name='*') existing row has a different non-nil BudgetID or
RateLimitID than the provider p; change the logic around the updates map (the
block referencing existing, p, updates, BudgetID, RateLimitID and the
tx.Table(...).Where("id = ?", existing.ID).Updates(updates).Error call) to first
detect conflicts and fail fast: if existing.BudgetID != nil && p.BudgetID != nil
&& *existing.BudgetID != *p.BudgetID return an error (include p.Name and both
IDs in the message), and likewise for RateLimitID, only proceed to backfill when
there is no conflict and then perform the Updates as before.
---
Duplicate comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 4348-4356: The customer-budget backfill check currently only logs
a warning when customer_budget_count is not 1; change it to mark the test suite
as failed by setting failed=1 when the query for governance_budgets (using
run_postgres_scalar) does not return 1, i.e. inside the else branch after
computing customer_budget_count; reference the existing variables
customer_budget_count and failed and replace or augment the log_warn call (or
add conditional gating if you prefer to preserve legacy behavior behind an
explicit legacy-schema check) so the test run fails on this regression instead
of merely warning.
- Around line 3940-3944: The script currently excludes governance_model_configs
entirely via the skip_tables variable which prevents any seeded row-value
checks; instead remove governance_model_configs from skip_tables and implement a
targeted relaxation: continue to run the seeded-row content assertions for
governance_model_configs (matching the seeded key pattern
"model-config-migration-test-*") but skip only the total-row-count assertion for
that table. Adjust the test logic that performs row-count vs. value comparisons
(the code paths using skip_tables and the row-count check) to special-case
governance_model_configs so count assertions are bypassed while seeded-row
equality checks still run.
- Around line 955-959: The update currently seeds
governance_model_pricing.additional_attributes with the default '{}' which won't
detect regressions; change the seeding to write a non-default JSON payload
(e.g., '{"seeded_test":"v1.5.6"}') when the script appends the UPDATE statements
for governance_model_pricing (the block that checks column_exists_postgres
"governance_model_pricing" "additional_attributes" and writes to
"$output_file"), and add an explicit post-migration comparison/assertion that
checks those specific seeded rows (by id) are preserved and match the
non-default payload in the PostgreSQL snapshot step; apply the same change to
the analogous block(s) referenced around the other occurrences with the same
pattern so the regression check actually detects clears/rewrites of
additional_attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e595539-13e8-497f-8850-bb62fad9c528
📒 Files selected for processing (2)
.github/workflows/scripts/run-migration-tests.shframework/configstore/migrations.go
akshaydeo
force-pushed
the
06-05-migration_test_fixes
branch
from
June 5, 2026 18:32
657ad7b to
c4d4fa4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 4348-4357: The check for the customer-budget backfill currently
only warns when customer_budget_count != "1", letting the migration regression
pass; update the block that computes customer_budget_count (the
run_postgres_scalar call and the if that compares it) to treat a missing
backfill as a test failure by logging an error and exiting non-zero (use
log_error or similar and exit 1) instead of calling log_warn, so that when
governance_budgets.id = 'budget-migration-test-3' does not have customer_id =
'customer-migration-test-1' the script fails the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a09d4ce8-8273-4e87-bcce-005d17535719
📒 Files selected for processing (2)
.github/workflows/scripts/run-migration-tests.shframework/configstore/migrations.go
akshaydeo
force-pushed
the
06-05-migration_test_fixes
branch
from
June 5, 2026 18:57
c4d4fa4 to
9439925
Compare
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)
.github/workflows/scripts/run-migration-tests.sh (1)
27-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the tag fetch from the repository root.
git fetch --tagsnow executes beforeSCRIPT_DIR/REPO_ROOTare resolved. If this script is invoked via an absolute or relative path from outside the checkout, it runs in the caller's CWD, hits “not a git repository”, and exits immediately underset -e.Suggested fix
-# Pull all the tags available -git fetch --tags - # Get the absolute path of the script directory if command -v readlink >/dev/null 2>&1 && readlink -f "$0" >/dev/null 2>&1; then SCRIPT_DIR="$(dirname "$(readlink -f "$0")")" else SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" fi # Repository root (3 levels up from .github/workflows/scripts) REPO_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd -P)" + +# Pull all the tags available +if ! (cd "$REPO_ROOT" && git fetch --tags >/dev/null 2>&1); then + log_warn "Could not refresh git tags; using locally available tags" +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/scripts/run-migration-tests.sh around lines 27 - 28, The script runs git fetch --tags before resolving SCRIPT_DIR/REPO_ROOT which can make it run outside the repo; update the script so that REPO_ROOT (or SCRIPT_DIR) is resolved first and then run git fetch --tags from that directory (e.g., cd to "$REPO_ROOT" before invoking git fetch --tags or invoke git with -C "$REPO_ROOT"); ensure the change targets the lines around the existing git fetch --tags invocation and references SCRIPT_DIR/REPO_ROOT so git always runs in the repository root.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/scripts/run-migration-tests.sh:
- Around line 27-28: The script runs git fetch --tags before resolving
SCRIPT_DIR/REPO_ROOT which can make it run outside the repo; update the script
so that REPO_ROOT (or SCRIPT_DIR) is resolved first and then run git fetch
--tags from that directory (e.g., cd to "$REPO_ROOT" before invoking git fetch
--tags or invoke git with -C "$REPO_ROOT"); ensure the change targets the lines
around the existing git fetch --tags invocation and references
SCRIPT_DIR/REPO_ROOT so git always runs in the repository root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b13e89a2-fa16-451f-9189-ea3f735a7ad9
📒 Files selected for processing (2)
.github/workflows/scripts/run-migration-tests.shframework/configstore/migrations.go
Contributor
Author
Merge activity
|
18 tasks
akshaydeo
added a commit
that referenced
this pull request
Jun 7, 2026
## Summary Fixes migration test failures introduced by the `add_customer_budgets_to_budgets_table` migration, which enforces single-ownership of budgets in v1.5.0-prerelease4+. The migration refuses to claim a budget already owned by another entity, so the test customer must reference a dedicated, unowned budget. Additionally adds migration test coverage for new v1.5.6 schema additions (`mcp_per_user_header_flows`, `mcp_per_user_header_credentials`, `config_mcp_clients.tls_config_json`, `config_mcp_clients.per_user_header_keys_json`, and `governance_model_pricing.additional_attributes`) and fixes a SQLite incompatibility in `DROP COLUMN IF EXISTS` statements. ## Changes - Introduced `budget-migration-test-3` as a dedicated budget for `customer-migration-test-1`, since `budget-migration-test-1` is already claimed by VK/provider/model-config folds and the ownership migration would reject it. - Added `generate_mcp_per_user_headers_insert_postgres` and `generate_mcp_per_user_headers_insert_sqlite` functions to seed `mcp_per_user_header_flows` and `mcp_per_user_header_credentials` rows when those tables exist (v1.5.6+). - Added dynamic column guards for `config_mcp_clients.tls_config_json` and `config_mcp_clients.per_user_header_keys_json` (both added in v1.5.6) in both Postgres and SQLite insert generators. - Added dynamic column guards for `governance_model_pricing.additional_attributes` (added in v1.5.6) in both Postgres and SQLite dynamic column appenders. - Introduced `dropColumnSQL` helper in `migrations.go` that emits `DROP COLUMN IF EXISTS` for Postgres and plain `DROP COLUMN` for SQLite (which does not support `IF EXISTS`), fixing a SQLite syntax error in `migrationAddMultiBudgetTables` and `migrationDropLegacyCalendarAlignedColumns`. - Fixed `migrationMigrateProviderGovernanceToModelConfigs` to insert wildcard model-config rows via an explicit column map rather than the live `TableModelConfig` struct, preventing GORM from including columns added by later migrations (e.g. `calendar_aligned`) before those columns exist. - Added SQLite foreign-key workaround to `migrationAddCustomerBudgetsToBudgetsTable`: pins to a single connection and disables `PRAGMA foreign_keys` before the migration transaction, then re-enables it afterward, mirroring the same pattern used in `migrationAddMultiBudgetTables`. - Added `governance_model_configs` to the snapshot comparison skip list in the migration test runner, since governance-folding migrations intentionally grow that table's row count. - Commented out `git fetch --tags` in the migration test script (was pulling remote tags unnecessarily in CI). ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh # Run migration tests locally ./.github/workflows/scripts/run-migration-tests.sh # Run Go tests go test ./framework/configstore/... ``` The migration test script will exercise both Postgres and SQLite upgrade paths across the last several versions. Confirm that: - All snapshot comparisons pass without false positives on `governance_model_configs`. - The `mcp_per_user_header_flows` and `mcp_per_user_header_credentials` rows are seeded correctly on v1.5.6+ schemas. - SQLite migrations no longer error on `DROP COLUMN IF EXISTS` syntax. - The customer budget ownership migration completes without rejecting `customer-migration-test-1`'s budget reference. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. Changes are limited to migration logic and test scaffolding. No auth, secrets, or PII handling is affected. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded migration tests with additional seed rows, per-user-header entries for PostgreSQL and SQLite, broader schema-compatibility checks, and extra budget ownership validations. * **Chores** * Improved migration test harness and version-selection logic; enhanced snapshot comparisons to handle seeded-row preservation when tables grow. * **Refactor** * Made migration operations idempotent and more DB-dialect-aware; safer schema cleanup and improved cross-database connection handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 11, 2026
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.
Summary
Fixes migration test failures introduced by the
add_customer_budgets_to_budgets_tablemigration, which enforces single-ownership of budgets in v1.5.0-prerelease4+. The migration refuses to claim a budget already owned by another entity, so the test customer must reference a dedicated, unowned budget. Additionally adds migration test coverage for new v1.5.6 schema additions (mcp_per_user_header_flows,mcp_per_user_header_credentials,config_mcp_clients.tls_config_json,config_mcp_clients.per_user_header_keys_json, andgovernance_model_pricing.additional_attributes) and fixes a SQLite incompatibility inDROP COLUMN IF EXISTSstatements.Changes
budget-migration-test-3as a dedicated budget forcustomer-migration-test-1, sincebudget-migration-test-1is already claimed by VK/provider/model-config folds and the ownership migration would reject it.generate_mcp_per_user_headers_insert_postgresandgenerate_mcp_per_user_headers_insert_sqlitefunctions to seedmcp_per_user_header_flowsandmcp_per_user_header_credentialsrows when those tables exist (v1.5.6+).config_mcp_clients.tls_config_jsonandconfig_mcp_clients.per_user_header_keys_json(both added in v1.5.6) in both Postgres and SQLite insert generators.governance_model_pricing.additional_attributes(added in v1.5.6) in both Postgres and SQLite dynamic column appenders.dropColumnSQLhelper inmigrations.gothat emitsDROP COLUMN IF EXISTSfor Postgres and plainDROP COLUMNfor SQLite (which does not supportIF EXISTS), fixing a SQLite syntax error inmigrationAddMultiBudgetTablesandmigrationDropLegacyCalendarAlignedColumns.migrationMigrateProviderGovernanceToModelConfigsto insert wildcard model-config rows via an explicit column map rather than the liveTableModelConfigstruct, preventing GORM from including columns added by later migrations (e.g.calendar_aligned) before those columns exist.migrationAddCustomerBudgetsToBudgetsTable: pins to a single connection and disablesPRAGMA foreign_keysbefore the migration transaction, then re-enables it afterward, mirroring the same pattern used inmigrationAddMultiBudgetTables.governance_model_configsto the snapshot comparison skip list in the migration test runner, since governance-folding migrations intentionally grow that table's row count.git fetch --tagsin the migration test script (was pulling remote tags unnecessarily in CI).Type of change
Affected areas
How to test
The migration test script will exercise both Postgres and SQLite upgrade paths across the last several versions. Confirm that:
governance_model_configs.mcp_per_user_header_flowsandmcp_per_user_header_credentialsrows are seeded correctly on v1.5.6+ schemas.DROP COLUMN IF EXISTSsyntax.customer-migration-test-1's budget reference.Breaking changes
Related issues
Security considerations
None. Changes are limited to migration logic and test scaffolding. No auth, secrets, or PII handling is affected.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Tests
Chores
Refactor