Skip to content

fix: reorder migrationReAddAllowDirectKeysColumn before config hash refresh migration and update schema with new fields - #3883

Merged
Pratham-Mishra04 merged 1 commit into
devfrom
05-29-fix_test_fixes
May 29, 2026
Merged

fix: reorder migrationReAddAllowDirectKeysColumn before config hash refresh migration and update schema with new fields#3883
Pratham-Mishra04 merged 1 commit into
devfrom
05-29-fix_test_fixes

Conversation

@Pratham-Mishra04

@Pratham-Mishra04 Pratham-Mishra04 commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes a migration ordering bug where migrationReAddAllowDirectKeysColumn was running after migrationRefreshConfigHashAfterMCPExternalServerURLRemoval, causing a "no such column: allow_direct_keys" failure on databases where the earlier drop migration had already run. The fix moves migrationReAddAllowDirectKeysColumn to execute before the config hash refresh migration. Additionally, several schema additions are included to keep the config schema in sync with current struct definitions.

Changes

  • Moved migrationReAddAllowDirectKeysColumn to run before migrationRefreshConfigHashAfterMCPExternalServerURLRemoval in the migration chain, since the hash refresh migration SELECTs config_client using the TableClientConfig struct which still declares allow_direct_keys
  • Added allow_direct_keys to the config schema under the client config section
  • Added source_id field to the team schema for optional external source identifier (e.g. SCIM group ID) mapping
  • Added blacklisted_models array field to the provider config schema for blocking specific models even when matched by allowed_models
  • Added per_user_headers as a valid auth_type enum value for MCP connections
  • Added per_user_header_keys array field to MCP connection config for specifying required caller-supplied headers when using per_user_headers auth
  • Added governance.virtual_keys exclusion for access_profile_id in the schema field test, as it is an enterprise-only field not present on the OSS TableVirtualKey

Type of change

  • Bug fix
  • Feature

Affected areas

  • Core (Go)
  • Transports (HTTP)

How to test

go test ./framework/configstore/...
go test ./transports/bifrost-http/lib/...

Verify that on a database where drop_allow_direct_keys_column_ddl has previously run, the full migration chain completes without a "no such column: allow_direct_keys" error. Confirm the config schema validates correctly against the updated config.schema.json.

Breaking changes

  • Yes
  • No

Related issues

The migration ordering issue would cause startup failures on any deployment that had previously run the column-drop migration before the config hash refresh migration was introduced.

Security considerations

The allow_direct_keys feature permits callers to bypass the registered key pool by supplying a raw provider API key via x-bf-direct-key. Ensure this field defaults to false and is only enabled intentionally, as it bypasses key management controls.

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

Summary by CodeRabbit

  • New Features
    • Added allow_direct_keys setting for direct API key usage
    • Added source_id field for mapping teams to external systems
    • Added blacklisted_models option to restrict model access in virtual key providers
    • Added per_user_headers authentication option for MCP client connections

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 04338cda-9c0b-4f9d-b8c3-6ebcbce76a58

📥 Commits

Reviewing files that changed from the base of the PR and between ff4dd4b and 65bdd6e.

📒 Files selected for processing (3)
  • framework/configstore/migrations.go
  • transports/bifrost-http/lib/config_test.go
  • transports/config.schema.json

📝 Walkthrough

Walkthrough

This PR extends the configuration schema with new authentication, identity-mapping, and model-filtering options while reordering migrations to resolve schema projection dependencies. Five new schema properties are added: allow_direct_keys for direct API key bypass, source_id for governance team external mapping, blacklisted_models for virtual key constraints, and MCP client per_user_headers authentication mode with required per_user_header_keys. Migrations are reordered to ensure allow_direct_keys column exists before the config-hash refresh query projects it. A test exclusion prevents schema drift validation of an enterprise-only field.

Changes

Configuration and Migration Updates

Layer / File(s) Summary
Configuration schema extensions
transports/config.schema.json
Schema adds client.allow_direct_keys (boolean, default false) to bypass registered key pool; governance.teams[].source_id (string) to map teams to external sources; blacklisted_models array to virtual key provider config for model-level blocking; MCP client auth_type enum extended with "per_user_headers" and new per_user_header_keys array for per-user header authorization.
Allow_direct_keys column migration reordering
framework/configstore/migrations.go
Migration sequence repositioned migrationReAddAllowDirectKeysColumn earlier, immediately after migrationAddCreatedByUserIDColumnForVirtualKeys and before migrationRefreshConfigHashAfterMCPExternalServerURLRemoval, ensuring the column exists when the refresh query projects TableClientConfig. Duplicate later invocation removed with explanatory comments documenting the ordering requirement.
Schema sync test exclusion for enterprise fields
transports/bifrost-http/lib/config_test.go
TestConfigSchemaSync excluded fields list extended to ignore governance.virtual_keys.access_profile_id, an enterprise-only assignment not present in OSS TableVirtualKey, preventing schema drift test failures.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • maximhq/bifrost#3639: Modifies the same migrationRefreshConfigHashAfterMCPExternalServerURLRemoval migration logic and its interaction with config_client column projections to address schema dependency issues.
  • maximhq/bifrost#3605: Both PRs modify triggerMigrations migration execution sequence to reorder migrations and satisfy schema-driven dependencies.

Suggested reviewers

  • danpiths
  • roroghost17

Poem

🐰 A schema grows with new keys and teams so bright,
Source IDs map and blacklists set things right,
Per-user headers auth, migrations dance in place,
Allow-direct-keys unlock the racing pace!

✨ 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-29-fix_test_fixes

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

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Pratham-Mishra04 commented May 29, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile-apps

greptile-apps Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Safe to merge — the migration reorder is a targeted correctness fix and all three migrations involved are idempotent.

The migration reorder directly addresses the startup crash on affected databases and both migration functions guard their DDL with existence checks, so already-healthy deployments are unaffected. Schema additions are additive and do not break existing consumers. The test exclusion is narrowly scoped to an enterprise-only field.

No files require special attention; the schema's missing conditional constraint for per_user_headers → per_user_header_keys was flagged in a prior review and is tracked outside this diff.

Important Files Changed

Filename Overview
framework/configstore/migrations.go Reorders migrationReAddAllowDirectKeysColumn before migrationRefreshConfigHashAfterMCPExternalServerURLRemoval to fix "no such column" crash on databases where the drop DDL had previously run. The reordering is correct and both migrations remain idempotent.
transports/config.schema.json Adds allow_direct_keys, source_id, blacklisted_models, per_user_headers enum value, and per_user_header_keys fields. The per_user_headers auth_type has no schema-level enforcement requiring per_user_header_keys to be non-empty, unlike the parallel oauth constraint (noted in a prior review comment).
transports/bifrost-http/lib/config_test.go Adds access_profile_id to the governance.virtual_keys exclusion list to prevent false failures on OSS builds where that enterprise-only field is absent from TableVirtualKey.

Reviews (3): Last reviewed commit: "fix: test fixes" | Re-trigger Greptile

Pratham-Mishra04 commented May 29, 2026

Copy link
Copy Markdown
Collaborator Author

Merge activity

  • May 29, 1:46 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 29, 1:52 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 29, 1:52 PM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

@Pratham-Mishra04
Pratham-Mishra04 changed the base branch from 05-29-docs_mcp_docs_improvements to graphite-base/3883 May 29, 2026 13:48
@Pratham-Mishra04
Pratham-Mishra04 changed the base branch from graphite-base/3883 to dev May 29, 2026 13:50
@Pratham-Mishra04
Pratham-Mishra04 merged commit 96f597d into dev May 29, 2026
12 of 14 checks passed
@Pratham-Mishra04
Pratham-Mishra04 deleted the 05-29-fix_test_fixes branch May 29, 2026 13:52
@coderabbitai coderabbitai Bot mentioned this pull request Jul 1, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants