fix: migration fix for drop columns for calendar aligned

[roroghost17]

Summary

Fixes a boot failure caused by GORM's Migrator.DropColumn performing a full SQLite table rebuild, which aborts when pre-existing foreign key violations are present. The migration that drops the legacy calendar_aligned columns from governance_budgets and governance_rate_limits is updated to use raw SQL instead, avoiding the rebuild entirely.

Changes

• Replaced gorm.Migrator.HasColumn / DropColumn calls in migrationDropLegacyCalendarAlignedColumns with raw ALTER TABLE ... DROP COLUMN IF EXISTS statements.
• Since these are unconstrained boolean columns with no data integrity implications, failures are now logged as warnings rather than returned as errors, preventing a hard boot failure in environments with FK violations.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

Run the migration against a SQLite database that has pre-existing foreign key violations and confirm the application boots successfully without erroring on the drop_legacy_calendar_aligned_columns migration step.

go test ./framework/configstore/...

Verify that the calendar_aligned column is dropped from both governance_budgets and governance_rate_limits when no FK violations are present, and that a warning is logged (rather than a fatal error) when the drop cannot be completed.

Screenshots/Recordings

N/A

Breaking changes

• Yes
• No

Related issues

N/A

Security considerations

None. The affected columns are unconstrained boolean fields with no auth, secrets, or PII implications.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Chores
  • Removed legacy calendar-aligned columns from governance budget and rate limit database tables as part of ongoing database maintenance. The migration process now handles column removal errors gracefully by logging them as warnings, ensuring the update completes without interruption.

Walkthrough

The migration migrationDropLegacyCalendarAlignedColumns now issues raw ALTER TABLE ... DROP COLUMN calendar_aligned statements against governance_budgets and governance_rate_limits, and converts any drop errors into logged warnings so the migration continues.

Changes

Legacy Column Migration Update

Layer / File(s)	Summary
Drop calendar_aligned migration resilience framework/configstore/migrations.go	Unconditional raw ALTER TABLE ... DROP COLUMN calendar_aligned statements replace the previous GORM-based conditional drops for governance_budgets and governance_rate_limits; drop failures are logged as warnings and do not abort the migration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• maximhq/bifrost#3452: Modifies migrationDropLegacyCalendarAlignedColumns behavior for dropping legacy calendar_aligned columns.
• maximhq/bifrost#3535: Related migrations around calendar_aligned backfill and usage across governance tables.
• maximhq/bifrost#3553: Switches migration steps to raw ALTER TABLE for calendar_aligned column handling.

Suggested reviewers

• danpiths

Poem

🐰 I hop through SQL, a gentle wink,
ALTER TABLE trims what we don't link.
Errors whispered, warnings stay,
Legacy fields hop away. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly references the main change: fixing the migration for dropping calendar_aligned columns, which matches the core issue described in the changeset.
Description check	✅ Passed	The description comprehensively covers all required sections including Summary, Changes, Type of change, Affected areas, How to test, Breaking changes, and Security considerations, with clear explanations of the fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-20-fix_migration_test_fix

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 @coderabbitai help to get the list of available commands and usage tips.}

[CLAassistant]

All committers have signed the CLA.

[roroghost17]

• fix: migration fix for drop columns for calendar aligned #3614 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 3/5

The migration prevents the boot crash it targets, but the raw SQL introduced will silently fail to drop the columns on SQLite in all environments — not just those with FK violations — because the IF EXISTS modifier is not valid SQLite syntax for ALTER TABLE DROP COLUMN.

On SQLite, ALTER TABLE … DROP COLUMN IF EXISTS is a parse error — the column is never dropped and the error is silently swallowed. The fix achieves its narrow goal (no hard boot failure) but leaves the column cleanup broken on every SQLite database, not just those with FK issues. The functional gap between intent and implementation on the primary target database warrants a closer look before merging.

framework/configstore/migrations.go — specifically the two raw SQL strings at lines 7700 and 7703

Important Files Changed

Filename	Overview
framework/configstore/migrations.go	Replaces GORM Migrator.DropColumn with raw SQL for the drop_legacy_calendar_aligned_columns migration; errors are now silently swallowed as warnings so boot never fails, but the raw SQL uses IF EXISTS which is not valid SQLite syntax, meaning the column drop silently fails on every SQLite run regardless of FK violations

_{Reviews (2): Last reviewed commit: "fix: migration test fix" | Re-trigger Greptile}

The merge-base changed after approval.

[coderabbitai]

🧹 Nitpick comments (1)

framework/configstore/migrations.go (1)

7696-7705: ⚡ Quick win

Avoid unconditional drop attempts to keep warning logs actionable.

This currently logs warnings for both real failures and expected “column already absent” cases. Since hasColumn already exists, pre-check before issuing DROP COLUMN so warnings represent actual failures.

♻️ Proposed change

 		Migrate: func(tx *gorm.DB) error {
 			tx = tx.WithContext(ctx)
 			// Use raw `ALTER TABLE ... DROP COLUMN IF EXISTS` instead of GORM's Migrator.DropColumn,
 			// which on SQLite does a full table rebuild that aborts on pre-existing FK violations;
 			// since these unconstrained boolean columns are safe to leave behind, we log a warning
 			// rather than fail boot.
-			if err := tx.Exec("ALTER TABLE governance_budgets DROP COLUMN calendar_aligned").Error; err != nil {
-				log.Printf("[Migration] warning: could not drop legacy calendar_aligned column from governance_budgets: %v", err)
+			if has, err := hasColumn(tx, "governance_budgets", "calendar_aligned"); err != nil {
+				log.Printf("[Migration] warning: could not introspect governance_budgets.calendar_aligned: %v", err)
+			} else if has {
+				if err := tx.Exec("ALTER TABLE governance_budgets DROP COLUMN calendar_aligned").Error; err != nil {
+					log.Printf("[Migration] warning: could not drop legacy calendar_aligned column from governance_budgets: %v", err)
+				}
 			}
-			if err := tx.Exec("ALTER TABLE governance_rate_limits DROP COLUMN calendar_aligned").Error; err != nil {
-				log.Printf("[Migration] warning: could not drop legacy calendar_aligned column from governance_rate_limits: %v", err)
+			if has, err := hasColumn(tx, "governance_rate_limits", "calendar_aligned"); err != nil {
+				log.Printf("[Migration] warning: could not introspect governance_rate_limits.calendar_aligned: %v", err)
+			} else if has {
+				if err := tx.Exec("ALTER TABLE governance_rate_limits DROP COLUMN calendar_aligned").Error; err != nil {
+					log.Printf("[Migration] warning: could not drop legacy calendar_aligned column from governance_rate_limits: %v", err)
+				}
 			}
 			return nil
 		},

🤖 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 `@framework/configstore/migrations.go` around lines 7696 - 7705, The migration
currently unconditionally runs tx.Exec("ALTER TABLE ... DROP COLUMN
calendar_aligned") and logs warnings even when the column is simply absent;
change it to first call hasColumn(tx, "governance_budgets", "calendar_aligned")
and only run the ALTER TABLE when hasColumn returns true, then log an error only
if the Exec fails; do the same for "governance_rate_limits" using hasColumn(tx,
"governance_rate_limits", "calendar_aligned") so warnings represent real
failures and not the expected missing-column case.

🤖 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.

Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 7696-7705: The migration currently unconditionally runs
tx.Exec("ALTER TABLE ... DROP COLUMN calendar_aligned") and logs warnings even
when the column is simply absent; change it to first call hasColumn(tx,
"governance_budgets", "calendar_aligned") and only run the ALTER TABLE when
hasColumn returns true, then log an error only if the Exec fails; do the same
for "governance_rate_limits" using hasColumn(tx, "governance_rate_limits",
"calendar_aligned") so warnings represent real failures and not the expected
missing-column case.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1bbbc576-6644-4bdb-b363-bb9f40dfd549

📥 Commits

Reviewing files that changed from the base of the PR and between 701538d and 8016d05.

📒 Files selected for processing (1)

• framework/configstore/migrations.go

[akshaydeo]

Merge activity

• May 20, 11:02 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 20, 11:03 AM UTC: @akshaydeo merged this pull request with Graphite.